| Message ID | 1346237818-9498-2-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 29, 2012 at 06:56:52PM +0800, Dong Aisheng wrote: > +config MFD_SYSCON > + bool "System Controller Register R/W Based on Regmap" If the driver only compiles and works for device tree probe, we should have the following? depends on OF > + select REGMAP_MMIO > + help > + Select this option to enable accessing system control registers > + via regmap. > + <snip> > +struct regmap *syscon_node_to_regmap(struct device_node *np) > +{ > + struct syscon *syscon; > + struct device *dev; > + > + dev = driver_find_device(&syscon_driver.driver, NULL, np, > + syscon_match); > + of_node_put(np); This looks a little unnatural to me. Function syscon_node_to_regmap becomes an API visible to clients, who might never know that np will be put inside the API. I'm saying the client may also call of_node_put to put the node they get. I think of_node_put should be moved out from here and put into syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. Regards, Shawn > + > + if (!dev) > + return ERR_PTR(-EPROBE_DEFER); > + > + syscon = dev_get_drvdata(dev); > + > + return syscon->regmap; > +} > +EXPORT_SYMBOL_GPL(syscon_node_to_regmap); > + > +struct regmap *syscon_regmap_lookup_by_compatible(const char *s) > +{ > + struct device_node *syscon_np; > + > + syscon_np = of_find_compatible_node(NULL, NULL, s); > + if (!syscon_np) > + return ERR_PTR(-ENODEV); > + > + return syscon_node_to_regmap(syscon_np); > +} > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); > + > +struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, > + const char *property) > +{ > + struct device_node *syscon_np; > + > + syscon_np = of_parse_phandle(np, property, 0); > + if (!syscon_np) > + return ERR_PTR(-ENODEV); > + > + return syscon_node_to_regmap(syscon_np); > +} > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
On Fri, Aug 31, 2012 at 09:26:29AM +0800, Shawn Guo wrote: > On Wed, Aug 29, 2012 at 06:56:52PM +0800, Dong Aisheng wrote: > > +config MFD_SYSCON > > + bool "System Controller Register R/W Based on Regmap" > > If the driver only compiles and works for device tree probe, we should > have the following? > > depends on OF > Correct. For currently it only supports dt. > > + select REGMAP_MMIO > > + help > > + Select this option to enable accessing system control registers > > + via regmap. > > + > > <snip> > > > +struct regmap *syscon_node_to_regmap(struct device_node *np) > > +{ > > + struct syscon *syscon; > > + struct device *dev; > > + > > + dev = driver_find_device(&syscon_driver.driver, NULL, np, > > + syscon_match); > > + of_node_put(np); > > This looks a little unnatural to me. Function syscon_node_to_regmap > becomes an API visible to clients, who might never know that np will > be put inside the API. I'm saying the client may also call of_node_put > to put the node they get. We probably could add a comment here for the API to avoid this happen. > > I think of_node_put should be moved out from here and put into > syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. > I guess no, if you want to move of_node_put into syscon_regmap_lookup_by_phandle, then syscon_regmap_lookup_by_phandle has the same issue. Actually i had considered your concern when writing this API... The original purpose of doing like that is saving some duplicated 'of_node_put' and make the API easy to use. I searched the kernel dt code and found it existed some similar cases. e.g: of_irq_find_parent, of_get_next_parent So it looks to me that it may be usually to do like that for the cases that the conversion from a node to other thing since the client may only care about the things converted. For our case, it's regmap. So i can't think it make too much sense for all client driver have to write duplicated and meaningless 'of_node_put' code. Regards Dong Aisheng > > + > > + if (!dev) > > + return ERR_PTR(-EPROBE_DEFER); > > + > > + syscon = dev_get_drvdata(dev); > > + > > + return syscon->regmap; > > +} > > +EXPORT_SYMBOL_GPL(syscon_node_to_regmap); > > + > > +struct regmap *syscon_regmap_lookup_by_compatible(const char *s) > > +{ > > + struct device_node *syscon_np; > > + > > + syscon_np = of_find_compatible_node(NULL, NULL, s); > > + if (!syscon_np) > > + return ERR_PTR(-ENODEV); > > + > > + return syscon_node_to_regmap(syscon_np); > > +} > > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); > > + > > +struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, > > + const char *property) > > +{ > > + struct device_node *syscon_np; > > + > > + syscon_np = of_parse_phandle(np, property, 0); > > + if (!syscon_np) > > + return ERR_PTR(-ENODEV); > > + > > + return syscon_node_to_regmap(syscon_np); > > +} > > +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle);
On Mon, Sep 03, 2012 at 10:31:03AM +0800, Dong Aisheng wrote: > > > > I think of_node_put should be moved out from here and put into > > syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. > > > I guess no, if you want to move of_node_put into syscon_regmap_lookup_by_phandle, > then syscon_regmap_lookup_by_phandle has the same issue. I guess not. syscon_regmap_lookup_by_phandle itself calls of_parse_phandle, and that's where the refcount gets incremented, so syscon_regmap_lookup_by_phandle should be responsible for calling of_node_put to get the refcount decremented.
On Mon, Sep 03, 2012 at 11:09:01AM +0800, Shawn Guo wrote: > On Mon, Sep 03, 2012 at 10:31:03AM +0800, Dong Aisheng wrote: > > > > > > I think of_node_put should be moved out from here and put into > > > syscon_node_to_regmap and syscon_regmap_lookup_by_compatible. > > > > > I guess no, if you want to move of_node_put into syscon_regmap_lookup_by_phandle, > > then syscon_regmap_lookup_by_phandle has the same issue. > > I guess not. syscon_regmap_lookup_by_phandle itself calls of_parse_phandle, > and that's where the refcount gets incremented, so syscon_regmap_lookup_by_phandle > should be responsible for calling of_node_put to get the refcount decremented. > Yes, the of_node_put will be done in syscon_node_to_regmap which is called by syscon_regmap_lookup_by_phandle, The reason why we do it is as i said in my last reply. I think one known issue is that syscon_node_to_regmap may not be suitable to be used by the driver who still wants to use the regmap node after calling the syscon_node_to_regmap. I still do not find such using case but i'm not sure whether it may exist. Probably the safe way currently to do is just as you said: Not put the node in syscon_node_to_regmap and let user decide. Ok, i will update it. Regards Dong Aisheng
diff --git a/Documentation/devicetree/bindings/mfd/syscon.txt b/Documentation/devicetree/bindings/mfd/syscon.txt new file mode 100644 index 0000000..fe8150b --- /dev/null +++ b/Documentation/devicetree/bindings/mfd/syscon.txt @@ -0,0 +1,20 @@ +* System Controller Registers R/W driver + +System controller node represents a register region containing a set +of miscellaneous registers. The registers are not cohesive enough to +represent as any specific type of device. The typical use-case is for +some other node's driver, or platform-specific code, to acquire a +reference to the syscon node (e.g. by phandle, node path, or search +using a specific compatible value), interrogate the node (or associated +OS driver) to determine the location of the registers, and access the +registers directly. + +Required properties: +- compatible: Should contain "syscon". +- reg: the register region can be accessed from syscon + +Examples: +gpr: iomuxc-gpr@020e0000 { + compatible = "fsl,imx6q-iomuxc-gpr", "syscon"; + reg = <0x020e0000 0x38>; +}; diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index b1a1462..2b53ed8 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -993,6 +993,13 @@ config MFD_ANATOP MFD controller. This controller embeds regulator and thermal devices for Freescale i.MX platforms. +config MFD_SYSCON + bool "System Controller Register R/W Based on Regmap" + select REGMAP_MMIO + help + Select this option to enable accessing system control registers + via regmap. + config MFD_PALMAS bool "Support for the TI Palmas series chips" select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 79dd22d..8384bc9 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -131,4 +131,5 @@ obj-$(CONFIG_MFD_PALMAS) += palmas.o obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o obj-$(CONFIG_MFD_ANATOP) += anatop-mfd.o +obj-$(CONFIG_MFD_SYSCON) += syscon.o obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c new file mode 100644 index 0000000..561c2ae --- /dev/null +++ b/drivers/mfd/syscon.c @@ -0,0 +1,170 @@ +/* + * System Control Driver + * + * Copyright (C) 2012 Freescale Semiconductor, Inc. + * Copyright (C) 2012 Linaro Ltd. + * + * Author: Dong Aisheng <dong.aisheng@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/err.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +static struct platform_driver syscon_driver; + +struct syscon { + struct device *dev; + void __iomem *base; + struct regmap *regmap; +}; + +static int syscon_match(struct device *dev, void *data) +{ + struct syscon *syscon = dev_get_drvdata(dev); + struct device_node *dn = data; + + return (syscon->dev->of_node == dn) ? 1 : 0; +} + +struct regmap *syscon_node_to_regmap(struct device_node *np) +{ + struct syscon *syscon; + struct device *dev; + + dev = driver_find_device(&syscon_driver.driver, NULL, np, + syscon_match); + of_node_put(np); + + if (!dev) + return ERR_PTR(-EPROBE_DEFER); + + syscon = dev_get_drvdata(dev); + + return syscon->regmap; +} +EXPORT_SYMBOL_GPL(syscon_node_to_regmap); + +struct regmap *syscon_regmap_lookup_by_compatible(const char *s) +{ + struct device_node *syscon_np; + + syscon_np = of_find_compatible_node(NULL, NULL, s); + if (!syscon_np) + return ERR_PTR(-ENODEV); + + return syscon_node_to_regmap(syscon_np); +} +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_compatible); + +struct regmap *syscon_regmap_lookup_by_phandle(struct device_node *np, + const char *property) +{ + struct device_node *syscon_np; + + syscon_np = of_parse_phandle(np, property, 0); + if (!syscon_np) + return ERR_PTR(-ENODEV); + + return syscon_node_to_regmap(syscon_np); +} +EXPORT_SYMBOL_GPL(syscon_regmap_lookup_by_phandle); + +static const struct of_device_id of_syscon_match[] = { + { .compatible = "syscon", }, + { }, +}; + +static struct regmap_config syscon_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, +}; + +static int __devinit 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; + + syscon = devm_kzalloc(&pdev->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; + + syscon_regmap_config.max_register = res.end - res.start - 3; + syscon->regmap = devm_regmap_init_mmio(&pdev->dev, syscon->base, + &syscon_regmap_config); + if (IS_ERR(syscon->regmap)) { + dev_err(&pdev->dev, "regmap init failed\n"); + return PTR_ERR(syscon->regmap); + } + + syscon->dev = &pdev->dev; + platform_set_drvdata(pdev, syscon); + + dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n", + res.start, res.end); + + return 0; +} + +static int __devexit 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 struct platform_driver syscon_driver = { + .driver = { + .name = "syscon", + .owner = THIS_MODULE, + .of_match_table = of_syscon_match, + }, + .probe = syscon_probe, + .remove = __devexit_p(syscon_remove), +}; + +static int __init syscon_init(void) +{ + return platform_driver_register(&syscon_driver); +} +postcore_initcall(syscon_init); + +static void __exit syscon_exit(void) +{ + platform_driver_unregister(&syscon_driver); +} +module_exit(syscon_exit); + +MODULE_AUTHOR("Dong Aisheng <dong.aisheng@linaro.org>"); +MODULE_DESCRIPTION("System Control driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h new file mode 100644 index 0000000..6aeb6b8 --- /dev/null +++ b/include/linux/mfd/syscon.h @@ -0,0 +1,23 @@ +/* + * System Control Driver + * + * Copyright (C) 2012 Freescale Semiconductor, Inc. + * Copyright (C) 2012 Linaro Ltd. + * + * Author: Dong Aisheng <dong.aisheng@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __LINUX_MFD_SYSCON_H__ +#define __LINUX_MFD_SYSCON_H__ + +extern struct regmap *syscon_node_to_regmap(struct device_node *np); +extern struct regmap *syscon_regmap_lookup_by_compatible(const char *s); +extern struct regmap *syscon_regmap_lookup_by_phandle( + struct device_node *np, + const char *property); +#endif /* __LINUX_MFD_SYSCON_H__ */