diff mbox

[1/7] mfd: add imx syscon driver based on regmap

Message ID 1345619928-15446-2-git-send-email-b29396@freescale.com (mailing list archive)
State New, archived
Headers show

Commit Message

Aisheng Dong Aug. 22, 2012, 7:18 a.m. UTC
From: Dong Aisheng <dong.aisheng@linaro.org>

Add regmap based imx syscon driver.
This is usually used for access misc bits in registers which does not belong
to a specific module, for example, IOMUXC GPR and ANATOP.
With this driver, we provide a standard API for client driver to call to
access registers which are registered into syscon.

Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
---
Currently it's just simply for IMX, however the driver really is not too
much imx specific.
If people want, we probably could extend it to support other platforms too.
---
 .../devicetree/bindings/mfd/imx-syscon.txt         |   11 +
 drivers/mfd/Kconfig                                |    8 +
 drivers/mfd/Makefile                               |    1 +
 drivers/mfd/imx-syscon.c                           |  193 ++++++++++++++++++++
 include/linux/mfd/imx-syscon.h                     |   22 +++
 5 files changed, 235 insertions(+), 0 deletions(-)

Comments

Richard Zhao Aug. 22, 2012, 8:29 a.m. UTC | #1
On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Add regmap based imx syscon driver.
> This is usually used for access misc bits in registers which does not belong
> to a specific module, for example, IOMUXC GPR and ANATOP.
> With this driver, we provide a standard API for client driver to call to
> access registers which are registered into syscon.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> Currently it's just simply for IMX, however the driver really is not too
> much imx specific.
> If people want, we probably could extend it to support other platforms too.
> ---
>  .../devicetree/bindings/mfd/imx-syscon.txt         |   11 +
>  drivers/mfd/Kconfig                                |    8 +
>  drivers/mfd/Makefile                               |    1 +
>  drivers/mfd/imx-syscon.c                           |  193 ++++++++++++++++++++
>  include/linux/mfd/imx-syscon.h                     |   22 +++
>  5 files changed, 235 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/imx-syscon.txt b/Documentation/devicetree/bindings/mfd/imx-syscon.txt
> new file mode 100644
> index 0000000..4a72994
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/imx-syscon.txt
> @@ -0,0 +1,11 @@
> +* Freescale IMX System Controller Registers R/W driver
> +
> +Required properties:
> +- compatible: Should contain "fsl,imx-syscon".
> +- reg: the register range can be access from imx-syscon
> +
> +Examples:
> +gpr: iomuxc-gpr@020e0000 {
> +	compatible = "fsl,imx6q-iomuxc", "fsl,imx-syscon";
why is it compatible with iomuxc?

> +	reg = <0x020e0000 0x38>;
> +};
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b1a1462..20a050e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -993,6 +993,14 @@ config MFD_ANATOP
>  	  MFD controller. This controller embeds regulator and
>  	  thermal devices for Freescale i.MX platforms.
>  
> +config MFD_IMX_SYSCON
> +        bool "Freescale i.MX System Controller Register R/W Based on Regmap"
> +        depends on ARCH_MXC
> +        select REGMAP_MMIO
> +        help
> +          Select this option to enable access Freescale i.MX system control
> +	  registers like iomuxc gpr and anatop 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..82c7ee1 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_IMX_SYSCON)	+= imx-syscon.o
>  obj-$(CONFIG_MFD_LM3533)	+= lm3533-core.o lm3533-ctrlbank.o
> diff --git a/drivers/mfd/imx-syscon.c b/drivers/mfd/imx-syscon.c
> new file mode 100644
> index 0000000..141b456
> --- /dev/null
> +++ b/drivers/mfd/imx-syscon.c
> @@ -0,0 +1,193 @@
> +/*
> + * Freescale IMX 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 imx_syscon_driver;
> +
> +struct imx_syscon {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct regmap *regmap;
> +};
> +
> +static int imx_syscon_match(struct device *dev, void *data)
> +{
> +	struct imx_syscon *syscon = dev_get_drvdata(dev);
> +	struct device_node *dn = data;
> +
> +	return (syscon->dev->of_node == dn) ? 1 : 0;
> +}
> +
> +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
For API function, is it better to use struct device rather not np?
 - it won't need to search dev in below code every time it access
   registers.
> +{
> +	struct device *dev;
> +	struct imx_syscon *syscon;
> +	int ret = 0;
> +
> +	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
> +				 imx_syscon_match);
> +	if (!dev)
> +		return -EPROBE_DEFER;
> +
> +	syscon = dev_get_drvdata(dev);
> +	ret = regmap_write(syscon->regmap, reg, val);
> +	if (ret)
> +		dev_err(dev, "failed to write regmap(%s) reg 0x%x (%d)\n",
> +			np->name, reg, ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_syscon_write);
> +
> +int imx_syscon_read(struct device_node *np, u32 reg, u32 *val)
> +{
> +	struct device *dev;
> +	struct imx_syscon *syscon;
> +	int ret = 0;
> +
> +	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
> +				 imx_syscon_match);
> +	if (!dev)
> +		return -EPROBE_DEFER;
> +
> +	syscon = dev_get_drvdata(dev);
> +	ret = regmap_read(syscon->regmap, reg, val);
> +	if (ret)
> +		dev_err(dev, "failed to read regmap(%s) reg 0x%x (%d)\n",
> +			np->name, reg, ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_syscon_read);
> +
> +int imx_syscon_update_bits(struct device_node *np, u32 reg,
> +					u32 mask, u32 val)
> +{
> +	struct device *dev;
> +	struct imx_syscon *syscon;
> +	int ret = 0;
> +
> +	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
> +				 imx_syscon_match);
> +	if (!dev)
> +		return -EPROBE_DEFER;
> +
> +	syscon = dev_get_drvdata(dev);
> +	ret = regmap_update_bits(syscon->regmap, reg, mask, val);
> +	if (ret)
> +		dev_err(dev, "failed to update regmap(%s) reg 0x%x (%d)\n",
> +			np->name, reg, ret);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(imx_syscon_update_bits);
> +
> +static const struct of_device_id of_imx_syscon_match[] = {
> +	{ .compatible = "fsl,imx-syscon", },
> +	{ },
> +};
> +
> +static struct regmap_config imx_syscon_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +};
> +
> +static int __devinit imx_syscon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct imx_syscon *syscon;
> +	struct resource res;
> +	int ret;
> +
> +	if (!np)
> +		return -ENOENT;
> +
> +	syscon = devm_kzalloc(&pdev->dev, sizeof(struct imx_syscon),
> +			    GFP_KERNEL);
> +	if (!syscon)
> +		return -ENOMEM;
> +
> +	syscon->base = of_iomap(np, 0);
no request? use devm_request_and_ioremap?

Thanks
Richard

> +	if (!syscon->base)
> +		return -EADDRNOTAVAIL;
> +
> +	ret = of_address_to_resource(np, 0, &res);
> +	if (ret)
> +		return ret;
> +
> +	imx_syscon_regmap_config.max_register = res.end - res.start - 3;
> +	syscon->regmap = devm_regmap_init_mmio(&pdev->dev, syscon->base,
> +					&imx_syscon_regmap_config);
> +	if (IS_ERR(syscon->regmap)) {
> +		dev_err(&pdev->dev, "regmap init failed\n");
> +		return PTR_ERR(syscon->regmap);
> +	}
> +
> +	regcache_cache_only(syscon->regmap, false);
> +
> +	dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
> +		res.start, res.end);
> +
> +	syscon->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, syscon);
> +
> +	return 0;
> +}
> +
> +static int __devexit imx_syscon_remove(struct platform_device *pdev)
> +{
> +	struct imx_syscon *syscon;
> +
> +	syscon = platform_get_drvdata(pdev);
> +	iounmap(syscon->base);
> +	platform_set_drvdata(pdev, NULL);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver imx_syscon_driver = {
> +	.driver = {
> +		.name = "imx-syscon",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_imx_syscon_match,
> +	},
> +	.probe		= imx_syscon_probe,
> +	.remove		= imx_syscon_remove,
> +};
> +
> +static int __init imx_syscon_init(void)
> +{
> +	return platform_driver_register(&imx_syscon_driver);
> +}
> +postcore_initcall(imx_syscon_init);
> +
> +static void __exit anatop_exit(void)
> +{
> +	platform_driver_unregister(&imx_syscon_driver);
> +}
> +module_exit(anatop_exit);
> +
> +MODULE_AUTHOR("Dong Aisheng <dong.aisheng@linaro.org>");
> +MODULE_DESCRIPTION("Freescale IMX System Control driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/imx-syscon.h b/include/linux/mfd/imx-syscon.h
> new file mode 100644
> index 0000000..be8b6db
> --- /dev/null
> +++ b/include/linux/mfd/imx-syscon.h
> @@ -0,0 +1,22 @@
> +/*
> + * Freescale IMX 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_IMX_SYSCON_H__
> +#define __LINUX_MFD_IMX_SYSCON_H__
> +
> +extern int imx_syscon_write(struct device_node *np, u32 reg, u32 val);
> +extern int imx_syscon_read(struct device_node *np, u32 reg, u32 *val);
> +extern int imx_syscon_update_bits(struct device_node *np, u32 reg,
> +					u32 mask, u32 val);
> +#endif /* __LINUX_MFD_IMX_SYSCON_H__ */
> -- 
> 1.7.0.4
>
Aisheng Dong Aug. 22, 2012, 10:57 a.m. UTC | #2
On Wed, Aug 22, 2012 at 04:29:41PM +0800, Zhao Richard-B20223 wrote:
> On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> > 
> > Add regmap based imx syscon driver.
> > This is usually used for access misc bits in registers which does not belong
> > to a specific module, for example, IOMUXC GPR and ANATOP.
> > With this driver, we provide a standard API for client driver to call to
> > access registers which are registered into syscon.
> > 
> > Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> > ---
> > Currently it's just simply for IMX, however the driver really is not too
> > much imx specific.
> > If people want, we probably could extend it to support other platforms too.
> > ---
> >  .../devicetree/bindings/mfd/imx-syscon.txt         |   11 +
> >  drivers/mfd/Kconfig                                |    8 +
> >  drivers/mfd/Makefile                               |    1 +
> >  drivers/mfd/imx-syscon.c                           |  193 ++++++++++++++++++++
> >  include/linux/mfd/imx-syscon.h                     |   22 +++
> >  5 files changed, 235 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/imx-syscon.txt b/Documentation/devicetree/bindings/mfd/imx-syscon.txt
> > new file mode 100644
> > index 0000000..4a72994
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/imx-syscon.txt
> > @@ -0,0 +1,11 @@
> > +* Freescale IMX System Controller Registers R/W driver
> > +
> > +Required properties:
> > +- compatible: Should contain "fsl,imx-syscon".
> > +- reg: the register range can be access from imx-syscon
> > +
> > +Examples:
> > +gpr: iomuxc-gpr@020e0000 {
> > +	compatible = "fsl,imx6q-iomuxc", "fsl,imx-syscon";
> why is it compatible with iomuxc?
> 
The first one usually is describing the device itself,
the second one is the required compatible string for using imx-sycon.

btw, it looks i'd better change "fsl,imx6q-iomuxc" to "fsl,imx6q-iomuxc-gpr"
since the later is the real case for us which is in patch 2/7.

> > +static int imx_syscon_match(struct device *dev, void *data)
> > +{
> > +	struct imx_syscon *syscon = dev_get_drvdata(dev);
> > +	struct device_node *dn = data;
> > +
> > +	return (syscon->dev->of_node == dn) ? 1 : 0;
> > +}
> > +
> > +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
> For API function, is it better to use struct device rather not np?
>  - it won't need to search dev in below code every time it access
>    registers.
The purpose is not require client driver to know the implementation details
of imx_syscon_{read/write} API, it's more easy to use since client only
needs pass the device node to which it wants to read/write.

For search dev, it doesn't look like a big issue since it only search devices
attached on the driver which is very quick.
And hide it in common API does not require every client driver to write
duplicated codes.

> > +static int __devinit imx_syscon_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct imx_syscon *syscon;
> > +	struct resource res;
> > +	int ret;
> > +
> > +	if (!np)
> > +		return -ENOENT;
> > +
> > +	syscon = devm_kzalloc(&pdev->dev, sizeof(struct imx_syscon),
> > +			    GFP_KERNEL);
> > +	if (!syscon)
> > +		return -ENOMEM;
> > +
> > +	syscon->base = of_iomap(np, 0);
> no request? use devm_request_and_ioremap?
> 
The io space registered into imx-sycon may be overlapped with other device,
e.g. iomuxc gpr overlapped with iomuxc. So we do not request it here.
There are also some exist examples, imx28 pinctrl with gpio devices contained.

Regards
Dong Aisheng
Mark Brown Aug. 22, 2012, 4:02 p.m. UTC | #3
On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:

> From: Dong Aisheng <dong.aisheng@linaro.org>

> Add regmap based imx syscon driver.

Nice to see more regmap-mmio usage!

Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

from a regmap point of view.

> +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
> +{
> +	struct device *dev;
> +	struct imx_syscon *syscon;
> +	int ret = 0;
> +
> +	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
> +				 imx_syscon_match);
> +	if (!dev)
> +		return -EPROBE_DEFER;
> +
> +	syscon = dev_get_drvdata(dev);
> +	ret = regmap_write(syscon->regmap, reg, val);

It'd be good to provide a way of retrieving the regmap so that drivers
for subsystems with generic regmap code could use the framework features
(regulator is one example that I just mentioned in my other mail).
Stephen Warren Aug. 23, 2012, 5:16 a.m. UTC | #4
On 08/22/2012 04:57 AM, Dong Aisheng wrote:
> On Wed, Aug 22, 2012 at 04:29:41PM +0800, Zhao Richard-B20223 wrote:
>> On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
>>> Add regmap based imx syscon driver.
>>> This is usually used for access misc bits in registers which does not belong
>>> to a specific module, for example, IOMUXC GPR and ANATOP.
>>> With this driver, we provide a standard API for client driver to call to
>>> access registers which are registered into syscon.

>>> +static int imx_syscon_match(struct device *dev, void *data)
>>> +{
>>> +	struct imx_syscon *syscon = dev_get_drvdata(dev);
>>> +	struct device_node *dn = data;
>>> +
>>> +	return (syscon->dev->of_node == dn) ? 1 : 0;
>>> +}
>>> +
>>> +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
>>
>> For API function, is it better to use struct device rather not np?
>>  - it won't need to search dev in below code every time it access
>>    registers.
>
> The purpose is not require client driver to know the implementation details
> of imx_syscon_{read/write} API, it's more easy to use since client only
> needs pass the device node to which it wants to read/write.
> 
> For search dev, it doesn't look like a big issue since it only search devices
> attached on the driver which is very quick.
> And hide it in common API does not require every client driver to write
> duplicated codes.

You could still implement a function:

struct device *imx_syscon_lookup(struct device_node *np)

... and require all clients to call that, and pass the dev to the other
functions. That'd still keep all the lookup code in one place, but
prevent it having to run every time, no matter how small it is.

I think such an API is required anyway, since client drivers need some
way to determine whether the imx_syscon driver is available yet, and if
not defer their probe until it is.

So, clients would do:

foo->syscon_dev = imx_syscon_lookup(np);
if (!foo->syscon_dev)
    return -EPROBE_DEFER;

rather than:

foo->syscon_np = np;

Not too much overhead/boiler-plate in each client driver.
Richard Zhao Aug. 23, 2012, 6:09 a.m. UTC | #5
On Wed, Aug 22, 2012 at 11:16:33PM -0600, Stephen Warren wrote:
> On 08/22/2012 04:57 AM, Dong Aisheng wrote:
> > On Wed, Aug 22, 2012 at 04:29:41PM +0800, Zhao Richard-B20223 wrote:
> >> On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
> >>> Add regmap based imx syscon driver.
> >>> This is usually used for access misc bits in registers which does not belong
> >>> to a specific module, for example, IOMUXC GPR and ANATOP.
> >>> With this driver, we provide a standard API for client driver to call to
> >>> access registers which are registered into syscon.
> 
> >>> +static int imx_syscon_match(struct device *dev, void *data)
> >>> +{
> >>> +	struct imx_syscon *syscon = dev_get_drvdata(dev);
> >>> +	struct device_node *dn = data;
> >>> +
> >>> +	return (syscon->dev->of_node == dn) ? 1 : 0;
> >>> +}
> >>> +
> >>> +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
> >>
> >> For API function, is it better to use struct device rather not np?
> >>  - it won't need to search dev in below code every time it access
> >>    registers.
> >
> > The purpose is not require client driver to know the implementation details
> > of imx_syscon_{read/write} API, it's more easy to use since client only
> > needs pass the device node to which it wants to read/write.
> > 
> > For search dev, it doesn't look like a big issue since it only search devices
> > attached on the driver which is very quick.
> > And hide it in common API does not require every client driver to write
> > duplicated codes.
> 
> You could still implement a function:
> 
> struct device *imx_syscon_lookup(struct device_node *np)
> 
> ... and require all clients to call that, and pass the dev to the other
> functions. That'd still keep all the lookup code in one place, but
> prevent it having to run every time, no matter how small it is.
> 
> I think such an API is required anyway, since client drivers need some
> way to determine whether the imx_syscon driver is available yet, and if
> not defer their probe until it is.
> 
> So, clients would do:
> 
> foo->syscon_dev = imx_syscon_lookup(np);
> if (!foo->syscon_dev)
>     return -EPROBE_DEFER;
> 
> rather than:
> 
> foo->syscon_np = np;
> 
> Not too much overhead/boiler-plate in each client driver.
I think it's good idea.

Thanks
Richard
Aisheng Dong Aug. 23, 2012, 7:06 a.m. UTC | #6
On Thu, Aug 23, 2012 at 01:16:33PM +0800, Stephen Warren wrote:
> On 08/22/2012 04:57 AM, Dong Aisheng wrote:
> > On Wed, Aug 22, 2012 at 04:29:41PM +0800, Zhao Richard-B20223 wrote:
> >> On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
> >>> Add regmap based imx syscon driver.
> >>> This is usually used for access misc bits in registers which does not belong
> >>> to a specific module, for example, IOMUXC GPR and ANATOP.
> >>> With this driver, we provide a standard API for client driver to call to
> >>> access registers which are registered into syscon.
> 
> >>> +static int imx_syscon_match(struct device *dev, void *data)
> >>> +{
> >>> +	struct imx_syscon *syscon = dev_get_drvdata(dev);
> >>> +	struct device_node *dn = data;
> >>> +
> >>> +	return (syscon->dev->of_node == dn) ? 1 : 0;
> >>> +}
> >>> +
> >>> +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
> >>
> >> For API function, is it better to use struct device rather not np?
> >>  - it won't need to search dev in below code every time it access
> >>    registers.
> >
> > The purpose is not require client driver to know the implementation details
> > of imx_syscon_{read/write} API, it's more easy to use since client only
> > needs pass the device node to which it wants to read/write.
> > 
> > For search dev, it doesn't look like a big issue since it only search devices
> > attached on the driver which is very quick.
> > And hide it in common API does not require every client driver to write
> > duplicated codes.
> 
> You could still implement a function:
> 
> struct device *imx_syscon_lookup(struct device_node *np)
> 
> ... and require all clients to call that, and pass the dev to the other
> functions. That'd still keep all the lookup code in one place, but
> prevent it having to run every time, no matter how small it is.
> 
> I think such an API is required anyway, since client drivers need some
> way to determine whether the imx_syscon driver is available yet, and if
> not defer their probe until it is.
> 
> So, clients would do:
> 
> foo->syscon_dev = imx_syscon_lookup(np);
> if (!foo->syscon_dev)
>     return -EPROBE_DEFER;
> 
> rather than:
> 
> foo->syscon_np = np;
> 
> Not too much overhead/boiler-plate in each client driver.
> 
Looks like a good idea.

Regards
Dong Aisheng
Aisheng Dong Aug. 23, 2012, 7:26 a.m. UTC | #7
On Thu, Aug 23, 2012 at 12:02:41AM +0800, Mark Brown wrote:
> On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
> 
> > From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> > Add regmap based imx syscon driver.
> 
> Nice to see more regmap-mmio usage!
> 
> Reviwed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
> from a regmap point of view.
> 
Thanks

> > +int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
> > +{
> > +	struct device *dev;
> > +	struct imx_syscon *syscon;
> > +	int ret = 0;
> > +
> > +	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
> > +				 imx_syscon_match);
> > +	if (!dev)
> > +		return -EPROBE_DEFER;
> > +
> > +	syscon = dev_get_drvdata(dev);
> > +	ret = regmap_write(syscon->regmap, reg, val);
> 
> It'd be good to provide a way of retrieving the regmap so that drivers
> for subsystems with generic regmap code could use the framework features
> (regulator is one example that I just mentioned in my other mail).

Do you mean something like:
regmap = syscon_regmap_dev_lookup(np, "fsl,anatop");
regmap_write(regmap, reg, val);

Then drivers can use generic regmap framework features rather than depend
on what imx-syscon implemented, is that correct?

Regards
Dong Aisheng
Mark Brown Aug. 23, 2012, 11:06 a.m. UTC | #8
On Thu, Aug 23, 2012 at 03:26:30PM +0800, Dong Aisheng wrote:
> On Thu, Aug 23, 2012 at 12:02:41AM +0800, Mark Brown wrote:

> > It'd be good to provide a way of retrieving the regmap so that drivers
> > for subsystems with generic regmap code could use the framework features
> > (regulator is one example that I just mentioned in my other mail).

> Do you mean something like:
> regmap = syscon_regmap_dev_lookup(np, "fsl,anatop");
> regmap_write(regmap, reg, val);

> Then drivers can use generic regmap framework features rather than depend
> on what imx-syscon implemented, is that correct?

Yes, this is mainly for cases where the subsystem has helper functions
that can work with regmap.
Aisheng Dong Aug. 24, 2012, 2:28 a.m. UTC | #9
On Thu, Aug 23, 2012 at 07:06:47PM +0800, Mark Brown wrote:
> On Thu, Aug 23, 2012 at 03:26:30PM +0800, Dong Aisheng wrote:
> > On Thu, Aug 23, 2012 at 12:02:41AM +0800, Mark Brown wrote:
> 
> > > It'd be good to provide a way of retrieving the regmap so that drivers
> > > for subsystems with generic regmap code could use the framework features
> > > (regulator is one example that I just mentioned in my other mail).
> 
> > Do you mean something like:
> > regmap = syscon_regmap_dev_lookup(np, "fsl,anatop");
> > regmap_write(regmap, reg, val);
> 
> > Then drivers can use generic regmap framework features rather than depend
> > on what imx-syscon implemented, is that correct?
> 
> Yes, this is mainly for cases where the subsystem has helper functions
> that can work with regmap.

Okay, then imx-syscon only implements regmap register mechanism and regmap
lookup mechanism, for accessors, client driver can directly use the generic
regmap API defined in include/linux/regmap.h.

Then it looks to me the driver is more like a generic feature which may also be
needed for other SoCs, IIRC, Tegra ahb and ux500 PRCMU,

Do you think if we should implement it in a more generic way at first?
e.g, drop 'imx-' prefix first.

Linus,
You're the first guy to raise the idea that we could implement a syscon
framework to generic register access, what's your comment on this?

Regards
Dong Aisheng
Shawn Guo Aug. 24, 2012, 6:43 a.m. UTC | #10
On Wed, Aug 22, 2012 at 03:18:42PM +0800, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@linaro.org>
> 
> Add regmap based imx syscon driver.
> This is usually used for access misc bits in registers which does not belong
> to a specific module, for example, IOMUXC GPR and ANATOP.
> With this driver, we provide a standard API for client driver to call to
> access registers which are registered into syscon.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@linaro.org>
> ---
> Currently it's just simply for IMX, however the driver really is not too
> much imx specific.
> If people want, we probably could extend it to support other platforms too.

Right.  I do not see anything IMX specific there.  We should probably
at least give the driver a generic name from day one.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/mfd/imx-syscon.txt b/Documentation/devicetree/bindings/mfd/imx-syscon.txt
new file mode 100644
index 0000000..4a72994
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/imx-syscon.txt
@@ -0,0 +1,11 @@ 
+* Freescale IMX System Controller Registers R/W driver
+
+Required properties:
+- compatible: Should contain "fsl,imx-syscon".
+- reg: the register range can be access from imx-syscon
+
+Examples:
+gpr: iomuxc-gpr@020e0000 {
+	compatible = "fsl,imx6q-iomuxc", "fsl,imx-syscon";
+	reg = <0x020e0000 0x38>;
+};
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index b1a1462..20a050e 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -993,6 +993,14 @@  config MFD_ANATOP
 	  MFD controller. This controller embeds regulator and
 	  thermal devices for Freescale i.MX platforms.
 
+config MFD_IMX_SYSCON
+        bool "Freescale i.MX System Controller Register R/W Based on Regmap"
+        depends on ARCH_MXC
+        select REGMAP_MMIO
+        help
+          Select this option to enable access Freescale i.MX system control
+	  registers like iomuxc gpr and anatop 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..82c7ee1 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_IMX_SYSCON)	+= imx-syscon.o
 obj-$(CONFIG_MFD_LM3533)	+= lm3533-core.o lm3533-ctrlbank.o
diff --git a/drivers/mfd/imx-syscon.c b/drivers/mfd/imx-syscon.c
new file mode 100644
index 0000000..141b456
--- /dev/null
+++ b/drivers/mfd/imx-syscon.c
@@ -0,0 +1,193 @@ 
+/*
+ * Freescale IMX 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 imx_syscon_driver;
+
+struct imx_syscon {
+	struct device *dev;
+	void __iomem *base;
+	struct regmap *regmap;
+};
+
+static int imx_syscon_match(struct device *dev, void *data)
+{
+	struct imx_syscon *syscon = dev_get_drvdata(dev);
+	struct device_node *dn = data;
+
+	return (syscon->dev->of_node == dn) ? 1 : 0;
+}
+
+int imx_syscon_write(struct device_node *np, u32 reg, u32 val)
+{
+	struct device *dev;
+	struct imx_syscon *syscon;
+	int ret = 0;
+
+	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
+				 imx_syscon_match);
+	if (!dev)
+		return -EPROBE_DEFER;
+
+	syscon = dev_get_drvdata(dev);
+	ret = regmap_write(syscon->regmap, reg, val);
+	if (ret)
+		dev_err(dev, "failed to write regmap(%s) reg 0x%x (%d)\n",
+			np->name, reg, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(imx_syscon_write);
+
+int imx_syscon_read(struct device_node *np, u32 reg, u32 *val)
+{
+	struct device *dev;
+	struct imx_syscon *syscon;
+	int ret = 0;
+
+	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
+				 imx_syscon_match);
+	if (!dev)
+		return -EPROBE_DEFER;
+
+	syscon = dev_get_drvdata(dev);
+	ret = regmap_read(syscon->regmap, reg, val);
+	if (ret)
+		dev_err(dev, "failed to read regmap(%s) reg 0x%x (%d)\n",
+			np->name, reg, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(imx_syscon_read);
+
+int imx_syscon_update_bits(struct device_node *np, u32 reg,
+					u32 mask, u32 val)
+{
+	struct device *dev;
+	struct imx_syscon *syscon;
+	int ret = 0;
+
+	dev = driver_find_device(&imx_syscon_driver.driver, NULL, np,
+				 imx_syscon_match);
+	if (!dev)
+		return -EPROBE_DEFER;
+
+	syscon = dev_get_drvdata(dev);
+	ret = regmap_update_bits(syscon->regmap, reg, mask, val);
+	if (ret)
+		dev_err(dev, "failed to update regmap(%s) reg 0x%x (%d)\n",
+			np->name, reg, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(imx_syscon_update_bits);
+
+static const struct of_device_id of_imx_syscon_match[] = {
+	{ .compatible = "fsl,imx-syscon", },
+	{ },
+};
+
+static struct regmap_config imx_syscon_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
+
+static int __devinit imx_syscon_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx_syscon *syscon;
+	struct resource res;
+	int ret;
+
+	if (!np)
+		return -ENOENT;
+
+	syscon = devm_kzalloc(&pdev->dev, sizeof(struct imx_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;
+
+	imx_syscon_regmap_config.max_register = res.end - res.start - 3;
+	syscon->regmap = devm_regmap_init_mmio(&pdev->dev, syscon->base,
+					&imx_syscon_regmap_config);
+	if (IS_ERR(syscon->regmap)) {
+		dev_err(&pdev->dev, "regmap init failed\n");
+		return PTR_ERR(syscon->regmap);
+	}
+
+	regcache_cache_only(syscon->regmap, false);
+
+	dev_info(dev, "syscon regmap start 0x%x end 0x%x registered\n",
+		res.start, res.end);
+
+	syscon->dev = &pdev->dev;
+	platform_set_drvdata(pdev, syscon);
+
+	return 0;
+}
+
+static int __devexit imx_syscon_remove(struct platform_device *pdev)
+{
+	struct imx_syscon *syscon;
+
+	syscon = platform_get_drvdata(pdev);
+	iounmap(syscon->base);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static struct platform_driver imx_syscon_driver = {
+	.driver = {
+		.name = "imx-syscon",
+		.owner = THIS_MODULE,
+		.of_match_table = of_imx_syscon_match,
+	},
+	.probe		= imx_syscon_probe,
+	.remove		= imx_syscon_remove,
+};
+
+static int __init imx_syscon_init(void)
+{
+	return platform_driver_register(&imx_syscon_driver);
+}
+postcore_initcall(imx_syscon_init);
+
+static void __exit anatop_exit(void)
+{
+	platform_driver_unregister(&imx_syscon_driver);
+}
+module_exit(anatop_exit);
+
+MODULE_AUTHOR("Dong Aisheng <dong.aisheng@linaro.org>");
+MODULE_DESCRIPTION("Freescale IMX System Control driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/imx-syscon.h b/include/linux/mfd/imx-syscon.h
new file mode 100644
index 0000000..be8b6db
--- /dev/null
+++ b/include/linux/mfd/imx-syscon.h
@@ -0,0 +1,22 @@ 
+/*
+ * Freescale IMX 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_IMX_SYSCON_H__
+#define __LINUX_MFD_IMX_SYSCON_H__
+
+extern int imx_syscon_write(struct device_node *np, u32 reg, u32 val);
+extern int imx_syscon_read(struct device_node *np, u32 reg, u32 *val);
+extern int imx_syscon_update_bits(struct device_node *np, u32 reg,
+					u32 mask, u32 val);
+#endif /* __LINUX_MFD_IMX_SYSCON_H__ */