| Message ID | 1328734286-30091-1-git-send-email-paul.liu@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Feb 09, 2012 at 04:51:25AM +0800, Ying-Chun Liu (PaulLiu) wrote: > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org> > > Anatop is a mfd chip embedded in Freescale i.MX6Q SoC. > Anatop provides regulators and thermal. > This driver handles the address space and the operation of the mfd device. > > Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org> > Cc: Samuel Ortiz <sameo@linux.intel.com> > --- > drivers/mfd/Kconfig | 6 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/anatop-mfd.c | 152 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/anatop.h | 39 +++++++++++ > 4 files changed, 198 insertions(+), 0 deletions(-) > create mode 100644 drivers/mfd/anatop-mfd.c > create mode 100644 include/linux/mfd/anatop.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index cd13e9f..4f71627 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC > Passage) chip. This chip embeds audio, battery, GPIO, etc. > devices used in Intel Medfield platforms. > > +config MFD_ANATOP > + bool "Support for Anatop" > + depends on SOC_IMX6Q > + help > + Select this option to enable Anatop MFD driver. > + > endmenu > endif > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index b953bab..8e11060 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o > obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o > obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o > obj-$(CONFIG_MFD_S5M_CORE) += s5m-core.o s5m-irq.o > +obj-$(CONFIG_MFD_ANATOP) += anatop-mfd.o > diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c > new file mode 100644 > index 0000000..58c6054 > --- /dev/null > +++ b/drivers/mfd/anatop-mfd.c > @@ -0,0 +1,152 @@ > +/* > + * Anatop MFD driver > + * > + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org> > + * Copyright (C) 2012 Linaro > + * > + * 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + * 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program; if not, write to the Free Software Foundation, Inc., > + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. > + * > + */ > + > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/of_address.h> > +#include <linux/mfd/anatop.h> > + > +static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int bits) > +{ > + u32 val; > + int mask; Nit: it may be nice to put a blank line here. > + if (bits == 32) > + mask = 0xff; Shouldn't it be ~0? > + else > + mask = (1 << bits) - 1; > + > + val = ioread32(adata->ioreg+addr); Why not just readl()? Nit: put space before and after '+'. > + val = (val >> bit_shift) & mask; Nit: it may be nice to put a blank line here. > + return val; > +} > + > +static void anatop_write(struct anatop *adata, u32 addr, int bit_shift, > + int bits, u32 data) > +{ > + u32 val; > + int mask; > + if (bits == 32) > + mask = 0xff; > + else > + mask = (1 << bits) - 1; > + > + val = ioread32(adata->ioreg+addr) & ~(mask << bit_shift); > + iowrite32((data << bit_shift) | val, adata->ioreg); Same comments on anatop_read apply on anatop_write here. > +} > + > +static const struct of_device_id of_anatop_regulator_match[] = { A better naming of of_anatop_regulator_match, since it covers not only regulator but also thermal as below? > + { > + .compatible = "fsl,anatop-regulator", > + }, Nit: since you only have one line here, it could be just: { .compatible = "fsl,anatop-regulator", }, > + { > + .compatible = "fsl,anatop-thermal", > + }, > + { }, > +}; > + > +static int of_anatop_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + u64 ofaddr; > + u64 ofsize; > + void *ioreg; > + struct anatop *drvdata; > + int ret = 0; > + const __be32 *rval; > + --- > + rval = of_get_address(np, 0, &ofsize, NULL); > + if (rval) > + ofaddr = be32_to_cpu(*rval); > + else > + return -EINVAL; > + > + ioreg = ioremap(ofaddr, ofsize); --- Above lines can just be of_iomap(np, 0); > + if (!ioreg) > + return -EINVAL; > + drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL); > + if (!drvdata) > + return -EINVAL; > + drvdata->ioreg = ioreg; > + drvdata->read = anatop_read; > + drvdata->write = anatop_write; > + platform_set_drvdata(pdev, drvdata); > + of_platform_bus_probe(np, of_anatop_regulator_match, dev); Nit: it may be nice to put a blank line here. > + return ret; The 'ret' is used nowhere, so the above line can just be: return 0; > +} > + > +static int of_anatop_remove(struct platform_device *pdev) > +{ Nothing to do here? At least iounmap? > + return 0; > +} > + > +static const struct of_device_id of_anatop_match[] = { > + { > + .compatible = "fsl,imx6q-anatop", > + }, { .compatible = "fsl,imx6q-anatop", }, > + { }, > +}; > + > +static struct platform_driver anatop_of_driver = { > + .driver = { > + .name = "anatop-mfd", > + .owner = THIS_MODULE, > + .of_match_table = of_anatop_match, > + }, > + .probe = of_anatop_probe, > + .remove = of_anatop_remove, > +}; > + > +static int __init anatop_init(void) > +{ > + int ret; > + ret = platform_driver_register(&anatop_of_driver); > + return ret; return platform_driver_register(&anatop_of_driver); > +} > + > +static void __exit anatop_exit(void) > +{ > + platform_driver_unregister(&anatop_of_driver); > +} > + > +postcore_initcall(anatop_init); > +module_exit(anatop_exit); > + > +MODULE_AUTHOR("Ying-Chun Liu (PaulLiu)"); Missing your email address here. > +MODULE_DESCRIPTION("ANATOP MFD driver"); > +MODULE_LICENSE("GPL"); "GPL v2" for better? > diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h > new file mode 100644 > index 0000000..4425538 > --- /dev/null > +++ b/include/linux/mfd/anatop.h > @@ -0,0 +1,39 @@ > +/* > + * anatop.h - Anatop MFD driver > + * > + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paulliu@debian.org> It would nice to use a consistent email address through the patch. Regards, Shawn > + * Copyright (C) 2012 Linaro > + * > + * 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. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, write to the Free Software > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA > + */ > + > +#ifndef __LINUX_MFD_ANATOP_H > +#define __LINUX_MFD_ANATOP_H > + > +/** > + * anatop - MFD data > + * @ioreg: ioremap register > + * @read: function to read bits from the device > + * @write: function to write bits to the device > + */ > +struct anatop { > + void *ioreg; > + u32 (*read) (struct anatop *adata, u32 addr, int bit_shift, int bits); > + void (*write) (struct anatop *adata, u32 addr, int bit_shift, > + int bits, u32 data); > + > +}; > + > +#endif /* __LINUX_MFD_ANATOP_H */ > -- > 1.7.8.3 >
Hi Paul, I didn't get patch #2, so I don't get to see how the read/write functions ar eused for example. On Thu, Feb 09, 2012 at 04:51:25AM +0800, Ying-Chun Liu (PaulLiu) wrote: > From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org> > > Anatop is a mfd chip embedded in Freescale i.MX6Q SoC. > Anatop provides regulators and thermal. > This driver handles the address space and the operation of the mfd device. A few comments here: > +static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int bits) > +{ > + u32 val; > + int mask; > + if (bits == 32) > + mask = 0xff; > + else > + mask = (1 << bits) - 1; > + > + val = ioread32(adata->ioreg+addr); > + val = (val >> bit_shift) & mask; > + return val; > +} > + > +static void anatop_write(struct anatop *adata, u32 addr, int bit_shift, > + int bits, u32 data) > +{ > + u32 val; > + int mask; > + if (bits == 32) > + mask = 0xff; > + else > + mask = (1 << bits) - 1; > + > + val = ioread32(adata->ioreg+addr) & ~(mask << bit_shift); > + iowrite32((data << bit_shift) | val, adata->ioreg); > +} Don't you need some sort of read/write atomic routine as well ? Locking would be needed then... > +static const struct of_device_id of_anatop_regulator_match[] = { > + { > + .compatible = "fsl,anatop-regulator", > + }, > + { > + .compatible = "fsl,anatop-thermal", > + }, > + { }, > +}; > + > +static int of_anatop_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + u64 ofaddr; > + u64 ofsize; > + void *ioreg; > + struct anatop *drvdata; > + int ret = 0; > + const __be32 *rval; > + > + rval = of_get_address(np, 0, &ofsize, NULL); > + if (rval) > + ofaddr = be32_to_cpu(*rval); > + else > + return -EINVAL; > + > + ioreg = ioremap(ofaddr, ofsize); > + if (!ioreg) > + return -EINVAL; > + drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL); > + if (!drvdata) > + return -EINVAL; > + drvdata->ioreg = ioreg; > + drvdata->read = anatop_read; > + drvdata->write = anatop_write; > + platform_set_drvdata(pdev, drvdata); > + of_platform_bus_probe(np, of_anatop_regulator_match, dev); > + return ret; > +} So it seems that your driver here does nothing but extending your device tree definition. Correct me if I'm wrong, aren't you trying to fix a broken device tree definition here ? Cheers, Samuel.
(2012年02月21日 19:17), Samuel Ortiz wrote: > Hi Paul, > > I didn't get patch #2, so I don't get to see how the read/write functions ar > eused for example. > Hi Samuel, Sorry for late reply. I've sent out v5 today. >> + ioreg = ioremap(ofaddr, ofsize); >> + if (!ioreg) >> + return -EINVAL; >> + drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL); >> + if (!drvdata) >> + return -EINVAL; >> + drvdata->ioreg = ioreg; >> + drvdata->read = anatop_read; >> + drvdata->write = anatop_write; >> + platform_set_drvdata(pdev, drvdata); >> + of_platform_bus_probe(np, of_anatop_regulator_match, dev); >> + return ret; >> +} > So it seems that your driver here does nothing but extending your device tree > definition. Correct me if I'm wrong, aren't you trying to fix a broken device > tree definition here ? > The driver here ioremap the addresses for anatop chip in i.MX6Q SoC. The addresses are shared by several regulators. Also thermal drivers are also in the same range. Here are examples of the anatop description in dts file anatop@020c8000 { compatible = "fsl,imx6q-anatop"; reg = <0x020c8000 0x1000>; interrupts = <0 49 0x04 0 54 0x04 0 127 0x04>; #address-cells = <1>; #size-cells = <1>; reg_vddpu: regulator-vddpu@140 { compatible = "fsl,anatop-regulator"; regulator-name = "vddpu"; regulator-min-microvolt = <725000>; regulator-max-microvolt = <1300000>; regulator-always-on; reg = <0x140 1>; vol-bit-shift = <9>; vol-bit-size = <5>; min-bit-val = <1>; min-voltage = <725000>; max-voltage = <1300000>; }; reg_vddcore: regulator-vddcore@140 { compatible = "fsl,anatop-regulator"; regulator-name = "vddcore"; regulator-min-microvolt = <725000>; regulator-max-microvolt = <1300000>; regulator-always-on; reg = <0x140 1>; vol-bit-shift = <0>; vol-bit-size = <5>; min-bit-val = <1>; min-voltage = <725000>; max-voltage = <1300000>; }; ... So both reg_vddpu and reg_vddcore are using the same address. Yours Sincerely, Paul
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index cd13e9f..4f71627 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC Passage) chip. This chip embeds audio, battery, GPIO, etc. devices used in Intel Medfield platforms. +config MFD_ANATOP + bool "Support for Anatop" + depends on SOC_IMX6Q + help + Select this option to enable Anatop MFD driver. + endmenu endif diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index b953bab..8e11060 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR) += tps65911-comparator.o obj-$(CONFIG_MFD_AAT2870_CORE) += aat2870-core.o obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o obj-$(CONFIG_MFD_S5M_CORE) += s5m-core.o s5m-irq.o +obj-$(CONFIG_MFD_ANATOP) += anatop-mfd.o diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c new file mode 100644 index 0000000..58c6054 --- /dev/null +++ b/drivers/mfd/anatop-mfd.c @@ -0,0 +1,152 @@ +/* + * Anatop MFD driver + * + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org> + * Copyright (C) 2012 Linaro + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, Inc., + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + * + */ + +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/of_address.h> +#include <linux/mfd/anatop.h> + +static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int bits) +{ + u32 val; + int mask; + if (bits == 32) + mask = 0xff; + else + mask = (1 << bits) - 1; + + val = ioread32(adata->ioreg+addr); + val = (val >> bit_shift) & mask; + return val; +} + +static void anatop_write(struct anatop *adata, u32 addr, int bit_shift, + int bits, u32 data) +{ + u32 val; + int mask; + if (bits == 32) + mask = 0xff; + else + mask = (1 << bits) - 1; + + val = ioread32(adata->ioreg+addr) & ~(mask << bit_shift); + iowrite32((data << bit_shift) | val, adata->ioreg); +} + +static const struct of_device_id of_anatop_regulator_match[] = { + { + .compatible = "fsl,anatop-regulator", + }, + { + .compatible = "fsl,anatop-thermal", + }, + { }, +}; + +static int of_anatop_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + u64 ofaddr; + u64 ofsize; + void *ioreg; + struct anatop *drvdata; + int ret = 0; + const __be32 *rval; + + rval = of_get_address(np, 0, &ofsize, NULL); + if (rval) + ofaddr = be32_to_cpu(*rval); + else + return -EINVAL; + + ioreg = ioremap(ofaddr, ofsize); + if (!ioreg) + return -EINVAL; + drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL); + if (!drvdata) + return -EINVAL; + drvdata->ioreg = ioreg; + drvdata->read = anatop_read; + drvdata->write = anatop_write; + platform_set_drvdata(pdev, drvdata); + of_platform_bus_probe(np, of_anatop_regulator_match, dev); + return ret; +} + +static int of_anatop_remove(struct platform_device *pdev) +{ + return 0; +} + +static const struct of_device_id of_anatop_match[] = { + { + .compatible = "fsl,imx6q-anatop", + }, + { }, +}; + +static struct platform_driver anatop_of_driver = { + .driver = { + .name = "anatop-mfd", + .owner = THIS_MODULE, + .of_match_table = of_anatop_match, + }, + .probe = of_anatop_probe, + .remove = of_anatop_remove, +}; + +static int __init anatop_init(void) +{ + int ret; + ret = platform_driver_register(&anatop_of_driver); + return ret; +} + +static void __exit anatop_exit(void) +{ + platform_driver_unregister(&anatop_of_driver); +} + +postcore_initcall(anatop_init); +module_exit(anatop_exit); + +MODULE_AUTHOR("Ying-Chun Liu (PaulLiu)"); +MODULE_DESCRIPTION("ANATOP MFD driver"); +MODULE_LICENSE("GPL"); diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h new file mode 100644 index 0000000..4425538 --- /dev/null +++ b/include/linux/mfd/anatop.h @@ -0,0 +1,39 @@ +/* + * anatop.h - Anatop MFD driver + * + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paulliu@debian.org> + * Copyright (C) 2012 Linaro + * + * 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. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + */ + +#ifndef __LINUX_MFD_ANATOP_H +#define __LINUX_MFD_ANATOP_H + +/** + * anatop - MFD data + * @ioreg: ioremap register + * @read: function to read bits from the device + * @write: function to write bits to the device + */ +struct anatop { + void *ioreg; + u32 (*read) (struct anatop *adata, u32 addr, int bit_shift, int bits); + void (*write) (struct anatop *adata, u32 addr, int bit_shift, + int bits, u32 data); + +}; + +#endif /* __LINUX_MFD_ANATOP_H */