| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1362997004.885485377@f319.mail.ru>
Date: Mon, 11 Mar 2013 14:16:44 +0400
From: Alexander Shiyan <shc_work@...l.ru>
To: Dong Aisheng <b29396@...escale.com>
Cc: linux-kernel@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Dong Aisheng <dong.aisheng@...aro.org>,
Samuel Ortiz <sameo@...ux.intel.com>,
Mark Brown <broonie@...nsource.wolfsonmicro.com>,
Thierry Reding <thierry.reding@...onic-design.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Stephen Warren <swarren@...dotorg.org>
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 <shc_work@...l.ru>
> > ---
> > drivers/mfd/Kconfig | 1 -
> > drivers/mfd/syscon.c | 77 +++++++++++++++++++++++++++-------------------
> > include/linux/mfd/syscon.h | 1 +
> > 3 files changed, 47 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 671f5b1..8fdd87e 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1070,7 +1070,6 @@ config MFD_STA2X11
> >
> > config MFD_SYSCON
> > bool "System Controller Register R/W Based on Regmap"
> > - depends on OF
> > select REGMAP_MMIO
> > help
> > Select this option to enable accessing system control registers
> > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> > index 674af14..e5b6be1 100644
> > --- a/drivers/mfd/syscon.c
> > +++ b/drivers/mfd/syscon.c
> > @@ -29,7 +29,7 @@ struct syscon {
> > struct regmap *regmap;
> > };
> >
> > -static int syscon_match(struct device *dev, void *data)
> > +static int syscon_match_node(struct device *dev, void *data)
> > {
> > struct device_node *dn = data;
> >
> > @@ -42,7 +42,7 @@ struct regmap *syscon_node_to_regmap(struct device_node *np)
> > struct device *dev;
> >
> > dev = driver_find_device(&syscon_driver.driver, NULL, np,
> > - syscon_match);
> > + syscon_match_node);
> > if (!dev)
> > return ERR_PTR(-EPROBE_DEFER);
> >
> > @@ -68,6 +68,34 @@ struct regmap *syscon_regmap_lookup_by_compatible(const char *s)
> > }
> > EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible);
> >
> > +static int syscon_match_pdevname(struct device *dev, void *data)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + const struct platform_device_id *id = platform_get_device_id(pdev);
> > +
> > + if (id)
> > + if (!strcmp(id->name, (const char *)data))
> > + return 1;
> > +
> > + return !strcmp(dev_name(dev), (const char *)data);
> > +}
> > +
> > +struct regmap *syscon_regmap_lookup_by_pdevname(const char *s)
> > +{
> > + struct device *dev;
> > + struct syscon *syscon;
> > +
> > + dev = driver_find_device(&syscon_driver.driver, NULL, (void *)s,
> > + syscon_match_pdevname);
> > + if (!dev)
> > + return ERR_PTR(-ENODEV);
>
> Should it be ERR_PTR(-EPROBE_DEFER)?
I have no idea what better using here. Think that is not so important
since we may have only one possible error code here.
> > +
> > + syscon = dev_get_drvdata(dev);
> > +
> > + return syscon->regmap;
> > +}
> > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_pdevname);
> > +
> > struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np,
> > const char *property)
> > {
> > @@ -99,30 +127,24 @@ static struct regmap_config syscon_regmap_config = {
> > static int syscon_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > - struct device_node *np = dev->of_node;
> > struct syscon *syscon;
> > - struct resource res;
> > - int ret;
> > -
> > - if (!np)
> > - return -ENOENT;
> > + struct resource *res;
> >
> > - syscon = devm_kzalloc(dev, sizeof(struct syscon),
> > - GFP_KERNEL);
> > + syscon = devm_kzalloc(dev, sizeof(struct syscon), GFP_KERNEL);
> > if (!syscon)
> > return -ENOMEM;
> >
> > - syscon->base = of_iomap(np, 0);
> > - if (!syscon->base)
> > - return -EADDRNOTAVAIL;
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENOENT;
> >
> > - ret = of_address_to_resource(np, 0, &res);
> > - if (ret)
> > - return ret;
> > + syscon->base = devm_ioremap(dev, res->start, resource_size(res));
> > + if (!syscon->base)
> > + return -ENOMEM;
> >
> > - syscon_regmap_config.max_register = res.end - res.start - 3;
> > + syscon_regmap_config.max_register = res->end - res->start - 3;
> > syscon->regmap = devm_regmap_init_mmio(dev, syscon->base,
> > - &syscon_regmap_config);
> > + &syscon_regmap_config);
>
> Unneeded change to me
>
> > if (IS_ERR(syscon->regmap)) {
> > dev_err(dev, "regmap init failed\n");
> > return PTR_ERR(syscon->regmap);
> > @@ -130,22 +152,15 @@ static int syscon_probe(struct platform_device *pdev)
> >
> > platform_set_drvdata(pdev, syscon);
> >
> > - dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
> > - res.start, res.end);
> > + dev_info(dev, "regmap 0x%x-0x%x registered\n", res->start, res->end);
>
> Doesn't with a 'syscon' prefix look better?
Prefix is added by dev_info, so no need to print it twice.
> >
> > return 0;
> > }
> >
> > -static int syscon_remove(struct platform_device *pdev)
> > -{
> > - struct syscon *syscon;
> > -
> > - syscon = platform_get_drvdata(pdev);
> > - iounmap(syscon->base);
> > - platform_set_drvdata(pdev, NULL);
> > -
> > - return 0;
> > -}
> > +static const struct platform_device_id syscon_ids[] = {
> > + { "syscon", },
> > + { }
> > +};
> >
> > static struct platform_driver syscon_driver = {
> > .driver = {
> > @@ -154,7 +169,7 @@ static struct platform_driver syscon_driver = {
> > .of_match_table = of_syscon_match,
> > },
> > .probe = syscon_probe,
> > - .remove = syscon_remove,
>
> So you reply on the managed resource to address the resource leak issue
> raised by Dmitry on doing manually unbind here, right?
Yes. We are killing "remove"-entry since all our resources are "managed" now.
> this looks ok to me.
>
> So, besides above minor comments,
> Acked-by: Dong Aisheng <dong.aisheng@...aro.org>
>
> Regards
> Dong Aisheng
>
> > + .id_table = syscon_ids,
> > };
> >
> > static int __init syscon_init(void)
> > diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
> > index 6aeb6b8..5c9ee6e 100644
> > --- a/include/linux/mfd/syscon.h
> > +++ b/include/linux/mfd/syscon.h
> > @@ -17,6 +17,7 @@
> >
> > extern struct regmap *syscon_node_to_regmap(struct device_node *np);
> > extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s);
> > +extern struct regmap *syscon_regmap_lookup_by_pdevname(const char *s);
> > extern struct regmap *syscon_regmap_lookup_by_phandle(
> > struct device_node *np,
> > const char *property);
> > --
> > 1.7.12.4
---
Powered by blists - more mailing lists