| Message ID | 1473344917-1524-3-git-send-email-quentin.schulz@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 08 Sep 2016, Quentin Schulz wrote: > The Allwinner SoCs all have an ADC that can also act as a touchscreen > controller and a thermal sensor. For now, only the ADC and the thermal > sensor drivers are probed by the MFD, the touchscreen controller support > will be added later. > > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> > --- > > v5: > - correct mail address, > > v4: > - rename files and variables from sunxi* to sun4i*, > - rename defines from SUNXI_* to SUN4I_* or SUN6I_*, > - remove TP in defines name, > - rename SUNXI_IRQ_* to SUN4I_GPADC_IRQ_* for consistency, > - use devm functions for regmap_add_irq_chip and mfd_add_devices, > - remove remove functions (now empty thanks to devm functions), > > v3: > - use defines in regmap_irq instead of hard coded BITs, > - use of_device_id data field to chose which MFD cells to add considering > the compatible responsible of the MFD probe, > - remove useless initializations, > - disable all interrupts before adding them to regmap_irqchip, > - add goto error label in probe, > - correct wrapping in header license, > - move defines from IIO driver to header, > - use GENMASK to limit the size of the variable passed to a macro, > - prefix register BIT defines with the name of the register, > - reorder defines, > > v2: > - add license headers, > - reorder alphabetically includes, > - add SUNXI_GPADC_ prefixes for defines, > > drivers/mfd/Kconfig | 15 ++++ > drivers/mfd/Makefile | 2 + > drivers/mfd/sun4i-gpadc-mfd.c | 174 ++++++++++++++++++++++++++++++++++++ > include/linux/mfd/sun4i-gpadc-mfd.h | 94 +++++++++++++++++++ > 4 files changed, 285 insertions(+) > create mode 100644 drivers/mfd/sun4i-gpadc-mfd.c > create mode 100644 include/linux/mfd/sun4i-gpadc-mfd.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 1bcf601..95b3c3e 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -29,6 +29,21 @@ config MFD_ACT8945A > linear regulators, along with a complete ActivePath battery > charger. > > +config MFD_SUN4I_GPADC > + tristate "Allwinner sunxi platforms' GPADC MFD driver" > + select MFD_CORE > + select REGMAP_MMIO > + depends on ARCH_SUNXI || COMPILE_TEST > + help > + Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC. > + This driver will only map the hardware interrupt and registers, you > + have to select individual drivers based on this MFD to be able to use > + the ADC or the thermal sensor. This will try to probe the ADC driver > + sun4i-gpadc-iio and the hwmon driver iio_hwmon. > + > + To compile this driver as a module, choose M here: the module will be > + called sun4i-gpadc-mfd. Drop the -mfd. > config MFD_AS3711 > bool "AMS AS3711" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 42a66e1..3b964d7 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -205,3 +205,5 @@ intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o > intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC) += intel_soc_pmic_bxtwc.o > obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o > obj-$(CONFIG_MFD_MT6397) += mt6397-core.o > + > +obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc-mfd.o > diff --git a/drivers/mfd/sun4i-gpadc-mfd.c b/drivers/mfd/sun4i-gpadc-mfd.c > new file mode 100644 > index 0000000..b499545 > --- /dev/null > +++ b/drivers/mfd/sun4i-gpadc-mfd.c Drop the -mfd. > @@ -0,0 +1,174 @@ > +/* ADC MFD core driver for sunxi platforms > + * > + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License version 2 as published by > + * the Free Software Foundation. > + */ > + > +#include <linux/interrupt.h> > +#include <linux/kernel.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/of_device.h> > +#include <linux/of_irq.h> > +#include <linux/regmap.h> > + > +#include <linux/mfd/sun4i-gpadc-mfd.h> > + > +static struct resource adc_resources[] = { > + { > + .name = "FIFO_DATA_PENDING", > + .start = SUN4I_GPADC_IRQ_FIFO_DATA, > + .end = SUN4I_GPADC_IRQ_FIFO_DATA, > + .flags = IORESOURCE_IRQ, > + }, { > + .name = "TEMP_DATA_PENDING", > + .start = SUN4I_GPADC_IRQ_TEMP_DATA, > + .end = SUN4I_GPADC_IRQ_TEMP_DATA, > + .flags = IORESOURCE_IRQ, > + }, > +}; Use the RES_IRQ_* defines. > +static const struct regmap_irq sun4i_gpadc_mfd_regmap_irq[] = { > + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_FIFO_DATA, 0, > + SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN), > + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_TEMP_DATA, 0, > + SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN), > +}; > + > +static const struct regmap_irq_chip sun4i_gpadc_mfd_regmap_irq_chip = { > + .name = "sun4i_gpadc_mfd_irq_chip", > + .status_base = SUN4I_GPADC_INT_FIFOS, > + .ack_base = SUN4I_GPADC_INT_FIFOS, > + .mask_base = SUN4I_GPADC_INT_FIFOC, > + .init_ack_masked = true, > + .mask_invert = true, > + .irqs = sun4i_gpadc_mfd_regmap_irq, > + .num_irqs = ARRAY_SIZE(sun4i_gpadc_mfd_regmap_irq), > + .num_regs = 1, > +}; > + > +static struct mfd_cell sun4i_gpadc_mfd_cells[] = { > + { > + .name = "sun4i-a10-gpadc-iio", > + .resources = adc_resources, > + .num_resources = ARRAY_SIZE(adc_resources), > + }, { > + .name = "iio_hwmon", > + } Single line please { .name = "iio_hwmon" } > +}; > + > +static struct mfd_cell sun5i_gpadc_mfd_cells[] = { > + { > + .name = "sun5i-a13-gpadc-iio", > + .resources = adc_resources, > + .num_resources = ARRAY_SIZE(adc_resources), > + }, { > + .name = "iio_hwmon", > + }, > +}; As above. > +static struct mfd_cell sun6i_gpadc_mfd_cells[] = { > + { > + .name = "sun6i-a31-gpadc-iio", > + .resources = adc_resources, > + .num_resources = ARRAY_SIZE(adc_resources), > + }, { > + .name = "iio_hwmon", > + }, > +}; As above. > +static const struct regmap_config sun4i_gpadc_mfd_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > + .fast_io = true, > +}; > + > +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { > + { > + .compatible = "allwinner,sun4i-a10-ts", > + .data = &sun4i_gpadc_mfd_cells, > + }, { > + .compatible = "allwinner,sun5i-a13-ts", > + .data = &sun5i_gpadc_mfd_cells, > + }, { > + .compatible = "allwinner,sun6i-a31-ts", > + .data = &sun6i_gpadc_mfd_cells, > + }, { /* sentinel */ } > +}; Don't mix OF and MFD functionality. Why don't you create a node for "iio_hwmon" and have platform_of_populate() do your bidding? > +static int sun4i_gpadc_mfd_probe(struct platform_device *pdev) Remove all mention of "mfd" from this file. (Accept the calls to the MFD API of course). > +{ > + struct sun4i_gpadc_mfd_dev *mfd_dev; > + struct resource *mem; > + const struct of_device_id *of_id; > + const struct mfd_cell *mfd_cells; > + unsigned int irq; > + int ret; > + > + of_id = of_match_node(sun4i_gpadc_mfd_of_match, pdev->dev.of_node); > + if (!of_id) > + return -EINVAL; > + > + mfd_dev = devm_kzalloc(&pdev->dev, sizeof(*mfd_dev), GFP_KERNEL); > + if (!mfd_dev) > + return -ENOMEM; > + > + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem); > + if (IS_ERR(mfd_dev->regs)) > + return PTR_ERR(mfd_dev->regs); > + > + mfd_dev->dev = &pdev->dev; > + dev_set_drvdata(mfd_dev->dev, mfd_dev); > + > + mfd_dev->regmap = devm_regmap_init_mmio(mfd_dev->dev, mfd_dev->regs, > + &sun4i_gpadc_mfd_regmap_config); > + if (IS_ERR(mfd_dev->regmap)) { > + ret = PTR_ERR(mfd_dev->regmap); > + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret); > + return ret; > + } > + > + /* Disable all interrupts */ > + regmap_write(mfd_dev->regmap, SUN4I_GPADC_INT_FIFOC, 0); > + > + irq = platform_get_irq(pdev, 0); > + ret = devm_regmap_add_irq_chip(&pdev->dev, mfd_dev->regmap, irq, > + IRQF_ONESHOT, 0, > + &sun4i_gpadc_mfd_regmap_irq_chip, > + &mfd_dev->regmap_irqc); > + if (ret) { > + dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret); > + return ret; > + } > + > + mfd_cells = of_id->data; > + ret = devm_mfd_add_devices(mfd_dev->dev, 0, mfd_cells, 2, NULL, 0, > + NULL); > + if (ret) { > + dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); Place this directly under the table. > +static struct platform_driver sun4i_gpadc_mfd_driver = { > + .driver = { > + .name = "sun4i-adc-mfd", > + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), > + }, > + .probe = sun4i_gpadc_mfd_probe, No .remove? > +}; > + > +module_platform_driver(sun4i_gpadc_mfd_driver); > + > +MODULE_DESCRIPTION("Allwinner sunxi platforms' GPADC MFD core driver"); > +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/mfd/sun4i-gpadc-mfd.h b/include/linux/mfd/sun4i-gpadc-mfd.h > new file mode 100644 > index 0000000..5cc7863 > --- /dev/null > +++ b/include/linux/mfd/sun4i-gpadc-mfd.h > @@ -0,0 +1,94 @@ > +/* Header of ADC MFD core driver for sunxi platforms > + * > + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.mfd> > + * > + * This program is free software; you can redistribute it and/or modify it under > + * the terms of the GNU General Public License version 2 as published by the > + * Free Software Foundation. > + */ > + > +#ifndef __SUN4I_GPADC_MFD__H__ > +#define __SUN4I_GPADC_MFD__H__ > + > +#define SUN4I_GPADC_CTRL0 0x00 > + > +#define SUN4I_GPADC_CTRL0_ADC_FIRST_DLY(x) ((GENMASK(7, 0) & (x)) << 24) > +#define SUN4I_GPADC_CTRL0_ADC_FIRST_DLY_MODE BIT(23) > +#define SUN4I_GPADC_CTRL0_ADC_CLK_SELECT BIT(22) > +#define SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(x) ((GENMASK(1, 0) & (x)) << 20) > +#define SUN4I_GPADC_CTRL0_FS_DIV(x) ((GENMASK(3, 0) & (x)) << 16) > +#define SUN4I_GPADC_CTRL0_T_ACQ(x) (GENMASK(15, 0) & (x)) > + > +#define SUN4I_GPADC_CTRL1 0x04 > + > +#define SUN4I_GPADC_CTRL1_STYLUS_UP_DEBOUNCE(x) ((GENMASK(7, 0) & (x)) << 12) > +#define SUN4I_GPADC_CTRL1_STYLUS_UP_DEBOUNCE_EN BIT(9) > +#define SUN4I_GPADC_CTRL1_TOUCH_PAN_CALI_EN BIT(6) > +#define SUN4I_GPADC_CTRL1_TP_DUAL_EN BIT(5) > +#define SUN4I_GPADC_CTRL1_TP_MODE_EN BIT(4) > +#define SUN4I_GPADC_CTRL1_TP_ADC_SELECT BIT(3) > +#define SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(2, 0) & (x)) > + > +/* TP_CTRL1 bits for sun6i SOCs */ > +#define SUN6I_GPADC_CTRL1_TOUCH_PAN_CALI_EN BIT(7) > +#define SUN6I_GPADC_CTRL1_TP_DUAL_EN BIT(6) > +#define SUN6I_GPADC_CTRL1_TP_MODE_EN BIT(5) > +#define SUN6I_GPADC_CTRL1_TP_ADC_SELECT BIT(4) > +#define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x)) > + > +#define SUN4I_GPADC_CTRL2 0x08 > + > +#define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28) > +#define SUN4I_GPADC_CTRL2_TP_MODE_SELECT(x) ((GENMASK(1, 0) & (x)) << 26) > +#define SUN4I_GPADC_CTRL2_PRE_MEA_EN BIT(24) > +#define SUN4I_GPADC_CTRL2_PRE_MEA_THRE_CNT(x) (GENMASK(23, 0) & (x)) > + > +#define SUN4I_GPADC_CTRL3 0x0c > + > +#define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2) > +#define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x)) > + > +#define SUN4I_GPADC_TPR 0x18 > + > +#define SUN4I_GPADC_TPR_TEMP_ENABLE BIT(16) > +#define SUN4I_GPADC_TPR_TEMP_PERIOD(x) (GENMASK(15, 0) & (x)) > + > +#define SUN4I_GPADC_INT_FIFOC 0x10 > + > +#define SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN BIT(18) > +#define SUN4I_GPADC_INT_FIFOC_TP_OVERRUN_IRQ_EN BIT(17) > +#define SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN BIT(16) > +#define SUN4I_GPADC_INT_FIFOC_TP_DATA_XY_CHANGE BIT(13) > +#define SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(x) ((GENMASK(4, 0) & (x)) << 8) > +#define SUN4I_GPADC_INT_FIFOC_TP_DATA_DRQ_EN BIT(7) > +#define SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH BIT(4) > +#define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1) > +#define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0) > + > +#define SUN4I_GPADC_INT_FIFOS 0x14 > + > +#define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18) > +#define SUN4I_GPADC_INT_FIFOS_FIFO_OVERRUN_PENDING BIT(17) > +#define SUN4I_GPADC_INT_FIFOS_FIFO_DATA_PENDING BIT(16) > +#define SUN4I_GPADC_INT_FIFOS_TP_IDLE_FLG BIT(2) > +#define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1) > +#define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0) > + > +#define SUN4I_GPADC_CDAT 0x1c > +#define SUN4I_GPADC_TEMP_DATA 0x20 > +#define SUN4I_GPADC_DATA 0x24 > + > +#define SUN4I_GPADC_IRQ_FIFO_DATA 0 > +#define SUN4I_GPADC_IRQ_TEMP_DATA 1 > + > +/* 10s delay before suspending the IP */ > +#define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 > + > +struct sun4i_gpadc_mfd_dev { > + struct device *dev; > + struct regmap *regmap; > + struct regmap_irq_chip_data *regmap_irqc; > + void __iomem *regs; It's *much* more common to call this 'base'. > +}; > + > +#endif -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On 12/09/2016 11:18, Lee Jones wrote: > On Thu, 08 Sep 2016, Quentin Schulz wrote: > [...] >> + To compile this driver as a module, choose M here: the module will be >> + called sun4i-gpadc-mfd. > > Drop the -mfd. > >> config MFD_AS3711 >> bool "AMS AS3711" >> select MFD_CORE >> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile >> index 42a66e1..3b964d7 100644 >> --- a/drivers/mfd/Makefile >> +++ b/drivers/mfd/Makefile >> @@ -205,3 +205,5 @@ intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o >> intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC) += intel_soc_pmic_bxtwc.o >> obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o >> obj-$(CONFIG_MFD_MT6397) += mt6397-core.o >> + >> +obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc-mfd.o >> diff --git a/drivers/mfd/sun4i-gpadc-mfd.c b/drivers/mfd/sun4i-gpadc-mfd.c >> new file mode 100644 >> index 0000000..b499545 >> --- /dev/null >> +++ b/drivers/mfd/sun4i-gpadc-mfd.c > > Drop the -mfd. > >> @@ -0,0 +1,174 @@ >> +/* ADC MFD core driver for sunxi platforms >> + * >> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License version 2 as published by >> + * the Free Software Foundation. >> + */ >> + >> +#include <linux/interrupt.h> >> +#include <linux/kernel.h> >> +#include <linux/mfd/core.h> >> +#include <linux/module.h> >> +#include <linux/of_device.h> >> +#include <linux/of_irq.h> >> +#include <linux/regmap.h> >> + >> +#include <linux/mfd/sun4i-gpadc-mfd.h> >> + >> +static struct resource adc_resources[] = { >> + { >> + .name = "FIFO_DATA_PENDING", >> + .start = SUN4I_GPADC_IRQ_FIFO_DATA, >> + .end = SUN4I_GPADC_IRQ_FIFO_DATA, >> + .flags = IORESOURCE_IRQ, >> + }, { >> + .name = "TEMP_DATA_PENDING", >> + .start = SUN4I_GPADC_IRQ_TEMP_DATA, >> + .end = SUN4I_GPADC_IRQ_TEMP_DATA, >> + .flags = IORESOURCE_IRQ, >> + }, >> +}; > > Use the RES_IRQ_* defines. > >> +static const struct regmap_irq sun4i_gpadc_mfd_regmap_irq[] = { >> + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_FIFO_DATA, 0, >> + SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN), >> + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_TEMP_DATA, 0, >> + SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN), >> +}; >> + >> +static const struct regmap_irq_chip sun4i_gpadc_mfd_regmap_irq_chip = { >> + .name = "sun4i_gpadc_mfd_irq_chip", >> + .status_base = SUN4I_GPADC_INT_FIFOS, >> + .ack_base = SUN4I_GPADC_INT_FIFOS, >> + .mask_base = SUN4I_GPADC_INT_FIFOC, >> + .init_ack_masked = true, >> + .mask_invert = true, >> + .irqs = sun4i_gpadc_mfd_regmap_irq, >> + .num_irqs = ARRAY_SIZE(sun4i_gpadc_mfd_regmap_irq), >> + .num_regs = 1, >> +}; >> + >> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = { >> + { >> + .name = "sun4i-a10-gpadc-iio", >> + .resources = adc_resources, >> + .num_resources = ARRAY_SIZE(adc_resources), >> + }, { >> + .name = "iio_hwmon", >> + } > > Single line please > > { .name = "iio_hwmon" } > + { + .name = "sun4i-a10-gpadc-iio", + .resources = adc_resources, + .num_resources = ARRAY_SIZE(adc_resources), + }, { .name = "iio_hwmon" } or + { + .name = "sun4i-a10-gpadc-iio", + .resources = adc_resources, + .num_resources = ARRAY_SIZE(adc_resources), + }, + { .name = "iio_hwmon" } ? >> +}; >> + >> +static struct mfd_cell sun5i_gpadc_mfd_cells[] = { >> + { >> + .name = "sun5i-a13-gpadc-iio", >> + .resources = adc_resources, >> + .num_resources = ARRAY_SIZE(adc_resources), >> + }, { >> + .name = "iio_hwmon", >> + }, >> +}; > > As above. > >> +static struct mfd_cell sun6i_gpadc_mfd_cells[] = { >> + { >> + .name = "sun6i-a31-gpadc-iio", >> + .resources = adc_resources, >> + .num_resources = ARRAY_SIZE(adc_resources), >> + }, { >> + .name = "iio_hwmon", >> + }, >> +}; > > As above. > >> +static const struct regmap_config sun4i_gpadc_mfd_regmap_config = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> + .fast_io = true, >> +}; >> + >> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { >> + { >> + .compatible = "allwinner,sun4i-a10-ts", >> + .data = &sun4i_gpadc_mfd_cells, >> + }, { >> + .compatible = "allwinner,sun5i-a13-ts", >> + .data = &sun5i_gpadc_mfd_cells, >> + }, { >> + .compatible = "allwinner,sun6i-a31-ts", >> + .data = &sun6i_gpadc_mfd_cells, >> + }, { /* sentinel */ } >> +}; > > Don't mix OF and MFD functionality. > > Why don't you create a node for "iio_hwmon" and have > platform_of_populate() do your bidding? > We are using a stable binding which we cannot modify. This means, the DT in its current state can only be modified to add features, which is not the case of this driver (it is a rewriting of an existing driver which uses the rtp node). >> +static int sun4i_gpadc_mfd_probe(struct platform_device *pdev) > > Remove all mention of "mfd" from this file. > > (Accept the calls to the MFD API of course). > [...] >> + >> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); > > Place this directly under the table. > >> +static struct platform_driver sun4i_gpadc_mfd_driver = { >> + .driver = { >> + .name = "sun4i-adc-mfd", >> + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), >> + }, >> + .probe = sun4i_gpadc_mfd_probe, > > No .remove? > No, everything in probe is handled with devm functions. [...] >> +struct sun4i_gpadc_mfd_dev { >> + struct device *dev; >> + struct regmap *regmap; >> + struct regmap_irq_chip_data *regmap_irqc; >> + void __iomem *regs; > > It's *much* more common to call this 'base'. > >> +}; >> + >> +#endif > ACK for everything else. Thanks, Quentin
On Mon, 12 Sep 2016, Maxime Ripard wrote: > On Mon, Sep 12, 2016 at 10:59:23AM +0100, Lee Jones wrote: > > > >> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { > > > >> + { > > > >> + .compatible = "allwinner,sun4i-a10-ts", > > > >> + .data = &sun4i_gpadc_mfd_cells, > > > >> + }, { > > > >> + .compatible = "allwinner,sun5i-a13-ts", > > > >> + .data = &sun5i_gpadc_mfd_cells, > > > >> + }, { > > > >> + .compatible = "allwinner,sun6i-a31-ts", > > > >> + .data = &sun6i_gpadc_mfd_cells, > > > >> + }, { /* sentinel */ } > > > >> +}; > > > > > > > > Don't mix OF and MFD functionality. > > > > > > > > Why don't you create a node for "iio_hwmon" and have > > > > platform_of_populate() do your bidding? > > > > > > > > > > We are using a stable binding which we cannot modify. This means, the DT > > > in its current state can only be modified to add features, which is not > > > the case of this driver (it is a rewriting of an existing driver which > > > uses the rtp node). > > > > Then use .data = <defined model ID> and set up a switch() in .probe(). > > Uh? Why? It just adds a non-standard indirection, while using > of_match_device is very standard, and used extensively in Linux. You still use of_match_device() to obtain the ID. The "don't mix DT with the MFD API" is there to prevent some of the nasty hacks I've seen previously. This particular example doesn't seem so bad, but it's a gateway to ridiculous hackery! -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On 12/09/2016 12:49, Lee Jones wrote: > On Mon, 12 Sep 2016, Maxime Ripard wrote: > >> On Mon, Sep 12, 2016 at 10:59:23AM +0100, Lee Jones wrote: >>>>>> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { >>>>>> + { >>>>>> + .compatible = "allwinner,sun4i-a10-ts", >>>>>> + .data = &sun4i_gpadc_mfd_cells, >>>>>> + }, { >>>>>> + .compatible = "allwinner,sun5i-a13-ts", >>>>>> + .data = &sun5i_gpadc_mfd_cells, >>>>>> + }, { >>>>>> + .compatible = "allwinner,sun6i-a31-ts", >>>>>> + .data = &sun6i_gpadc_mfd_cells, >>>>>> + }, { /* sentinel */ } >>>>>> +}; >>>>> >>>>> Don't mix OF and MFD functionality. >>>>> >>>>> Why don't you create a node for "iio_hwmon" and have >>>>> platform_of_populate() do your bidding? >>>>> >>>> >>>> We are using a stable binding which we cannot modify. This means, the DT >>>> in its current state can only be modified to add features, which is not >>>> the case of this driver (it is a rewriting of an existing driver which >>>> uses the rtp node). >>> >>> Then use .data = <defined model ID> and set up a switch() in .probe(). >> >> Uh? Why? It just adds a non-standard indirection, while using >> of_match_device is very standard, and used extensively in Linux. > > You still use of_match_device() to obtain the ID. > > The "don't mix DT with the MFD API" is there to prevent some of the > nasty hacks I've seen previously. This particular example doesn't > seem so bad, but it's a gateway to ridiculous hackery! > How am I supposed to get the .data without of_match_node then? What's more hackish in using .data field for specific data for each compatible than in using a random ID in .data and switching on it? The result is exactly the same, the switching case being more verbose and adding complexity to something that can be done in a straightforward manner. Quentin
On 12/09/2016 11:59, Lee Jones wrote: > On Mon, 12 Sep 2016, Quentin Schulz wrote: > >> On 12/09/2016 11:18, Lee Jones wrote: >>> On Thu, 08 Sep 2016, Quentin Schulz wrote: >>> >> [...] > > [...] > >>>> +++ b/drivers/mfd/sun4i-gpadc-mfd.c > > [...] > >>>> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = { >>>> + { >>>> + .name = "sun4i-a10-gpadc-iio", >>>> + .resources = adc_resources, >>>> + .num_resources = ARRAY_SIZE(adc_resources), >>>> + }, { >>>> + .name = "iio_hwmon", >>>> + } >>> >>> Single line please >>> >>> { .name = "iio_hwmon" } >>> >> >> + { >> + .name = "sun4i-a10-gpadc-iio", >> + .resources = adc_resources, >> + .num_resources = ARRAY_SIZE(adc_resources), >> + }, { .name = "iio_hwmon" } >> >> or >> >> + { >> + .name = "sun4i-a10-gpadc-iio", >> + .resources = adc_resources, >> + .num_resources = ARRAY_SIZE(adc_resources), >> + }, >> + { .name = "iio_hwmon" } >> >> ? > > The latter. > > [...] > >>>> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { >>>> + { >>>> + .compatible = "allwinner,sun4i-a10-ts", >>>> + .data = &sun4i_gpadc_mfd_cells, >>>> + }, { >>>> + .compatible = "allwinner,sun5i-a13-ts", >>>> + .data = &sun5i_gpadc_mfd_cells, >>>> + }, { >>>> + .compatible = "allwinner,sun6i-a31-ts", >>>> + .data = &sun6i_gpadc_mfd_cells, >>>> + }, { /* sentinel */ } >>>> +}; >>> >>> Don't mix OF and MFD functionality. >>> >>> Why don't you create a node for "iio_hwmon" and have >>> platform_of_populate() do your bidding? >>> >> >> We are using a stable binding which we cannot modify. This means, the DT >> in its current state can only be modified to add features, which is not >> the case of this driver (it is a rewriting of an existing driver which >> uses the rtp node). > > Then use .data = <defined model ID> and set up a switch() in .probe(). > >>>> +static int sun4i_gpadc_mfd_probe(struct platform_device *pdev) >>> >>> Remove all mention of "mfd" from this file. >>> >>> (Accept the calls to the MFD API of course). >>> >> [...] >>>> + >>>> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); >>> >>> Place this directly under the table. >>> >>>> +static struct platform_driver sun4i_gpadc_mfd_driver = { >>>> + .driver = { >>>> + .name = "sun4i-adc-mfd", >>>> + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), >>>> + }, >>>> + .probe = sun4i_gpadc_mfd_probe, >>> >>> No .remove? >>> >> >> No, everything in probe is handled with devm functions. > > Don't you need to undo the register write you did? > The regmap_write I use is there to disable all interrupts on hardware side before the irq_chip handles all interrupts by itself. The interrupts are not used in the MFD driver. Thus, I chose to disable the hardware interrupts in the remove function of drivers using the interrupts (only the IIO yet but the touchscreen driver later also which will be using a third interrupt). When the MFD driver is removed, the MFD cells will all be removed, thus calling their own remove functions, thus disabling hardware interrupts used in each driver. So the hardware interrupts disabling would be called twice. Quentin
On Mon, 12 Sep 2016, Quentin Schulz wrote: > On 12/09/2016 11:59, Lee Jones wrote: > > On Mon, 12 Sep 2016, Quentin Schulz wrote: > > > >> On 12/09/2016 11:18, Lee Jones wrote: > >>> On Thu, 08 Sep 2016, Quentin Schulz wrote: > >>> > >> [...] > > > > [...] > > > >>>> +++ b/drivers/mfd/sun4i-gpadc-mfd.c > > > > [...] > > > >>>> +static struct mfd_cell sun4i_gpadc_mfd_cells[] = { > >>>> + { > >>>> + .name = "sun4i-a10-gpadc-iio", > >>>> + .resources = adc_resources, > >>>> + .num_resources = ARRAY_SIZE(adc_resources), > >>>> + }, { > >>>> + .name = "iio_hwmon", > >>>> + } > >>> > >>> Single line please > >>> > >>> { .name = "iio_hwmon" } > >>> > >> > >> + { > >> + .name = "sun4i-a10-gpadc-iio", > >> + .resources = adc_resources, > >> + .num_resources = ARRAY_SIZE(adc_resources), > >> + }, { .name = "iio_hwmon" } > >> > >> or > >> > >> + { > >> + .name = "sun4i-a10-gpadc-iio", > >> + .resources = adc_resources, > >> + .num_resources = ARRAY_SIZE(adc_resources), > >> + }, > >> + { .name = "iio_hwmon" } > >> > >> ? > > > > The latter. > > > > [...] > > > >>>> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { > >>>> + { > >>>> + .compatible = "allwinner,sun4i-a10-ts", > >>>> + .data = &sun4i_gpadc_mfd_cells, > >>>> + }, { > >>>> + .compatible = "allwinner,sun5i-a13-ts", > >>>> + .data = &sun5i_gpadc_mfd_cells, > >>>> + }, { > >>>> + .compatible = "allwinner,sun6i-a31-ts", > >>>> + .data = &sun6i_gpadc_mfd_cells, > >>>> + }, { /* sentinel */ } > >>>> +}; > >>> > >>> Don't mix OF and MFD functionality. > >>> > >>> Why don't you create a node for "iio_hwmon" and have > >>> platform_of_populate() do your bidding? > >>> > >> > >> We are using a stable binding which we cannot modify. This means, the DT > >> in its current state can only be modified to add features, which is not > >> the case of this driver (it is a rewriting of an existing driver which > >> uses the rtp node). > > > > Then use .data = <defined model ID> and set up a switch() in .probe(). > > > >>>> +static int sun4i_gpadc_mfd_probe(struct platform_device *pdev) > >>> > >>> Remove all mention of "mfd" from this file. > >>> > >>> (Accept the calls to the MFD API of course). > >>> > >> [...] > >>>> + > >>>> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); > >>> > >>> Place this directly under the table. > >>> > >>>> +static struct platform_driver sun4i_gpadc_mfd_driver = { > >>>> + .driver = { > >>>> + .name = "sun4i-adc-mfd", > >>>> + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), > >>>> + }, > >>>> + .probe = sun4i_gpadc_mfd_probe, > >>> > >>> No .remove? > >>> > >> > >> No, everything in probe is handled with devm functions. > > > > Don't you need to undo the register write you did? > > > > The regmap_write I use is there to disable all interrupts on hardware > side before the irq_chip handles all interrupts by itself. The > interrupts are not used in the MFD driver. > > Thus, I chose to disable the hardware interrupts in the remove function > of drivers using the interrupts (only the IIO yet but the touchscreen > driver later also which will be using a third interrupt). When the MFD > driver is removed, the MFD cells will all be removed, thus calling their > own remove functions, thus disabling hardware interrupts used in each > driver. So the hardware interrupts disabling would be called twice. This does send some little alarm bells ringing. I'd normally expect the .remove function to undo everything you did in .probe. So, if you are disabling the IRQs from within the leaf drivers, shouldn't you be initialising them in the leaf driver's respective .probes? -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Mon, 12 Sep 2016, Quentin Schulz wrote: > On 12/09/2016 12:49, Lee Jones wrote: > > On Mon, 12 Sep 2016, Maxime Ripard wrote: > > > >> On Mon, Sep 12, 2016 at 10:59:23AM +0100, Lee Jones wrote: > >>>>>> +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { > >>>>>> + { > >>>>>> + .compatible = "allwinner,sun4i-a10-ts", > >>>>>> + .data = &sun4i_gpadc_mfd_cells, > >>>>>> + }, { > >>>>>> + .compatible = "allwinner,sun5i-a13-ts", > >>>>>> + .data = &sun5i_gpadc_mfd_cells, > >>>>>> + }, { > >>>>>> + .compatible = "allwinner,sun6i-a31-ts", > >>>>>> + .data = &sun6i_gpadc_mfd_cells, > >>>>>> + }, { /* sentinel */ } > >>>>>> +}; > >>>>> > >>>>> Don't mix OF and MFD functionality. > >>>>> > >>>>> Why don't you create a node for "iio_hwmon" and have > >>>>> platform_of_populate() do your bidding? > >>>>> > >>>> > >>>> We are using a stable binding which we cannot modify. This means, the DT > >>>> in its current state can only be modified to add features, which is not > >>>> the case of this driver (it is a rewriting of an existing driver which > >>>> uses the rtp node). > >>> > >>> Then use .data = <defined model ID> and set up a switch() in .probe(). > >> > >> Uh? Why? It just adds a non-standard indirection, while using > >> of_match_device is very standard, and used extensively in Linux. > > > > You still use of_match_device() to obtain the ID. > > > > The "don't mix DT with the MFD API" is there to prevent some of the > > nasty hacks I've seen previously. This particular example doesn't > > seem so bad, but it's a gateway to ridiculous hackery! > > How am I supposed to get the .data without of_match_node then? > What's more hackish in using .data field for specific data for each > compatible than in using a random ID in .data and switching on it? The > result is exactly the same, the switching case being more verbose and > adding complexity to something that can be done in a straightforward manner. I've already agreed that your implementation isn't terrible, but I'd still like to remain strict on the rules. Better still, can you can dynamically test which platform you're on, via a version register or similar? Failing that, see how everyone else does it: `git grep "\.data" -- drivers/mfd/` -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On Mon, 12 Sep 2016, Maxime Ripard wrote: > On Mon, Sep 12, 2016 at 02:56:55PM +0100, Lee Jones wrote: > > > >>> Then use .data = <defined model ID> and set up a switch() in .probe(). > > > >> > > > >> Uh? Why? It just adds a non-standard indirection, while using > > > >> of_match_device is very standard, and used extensively in Linux. > > > > > > > > You still use of_match_device() to obtain the ID. > > > > > > > > The "don't mix DT with the MFD API" is there to prevent some of the > > > > nasty hacks I've seen previously. This particular example doesn't > > > > seem so bad, but it's a gateway to ridiculous hackery! > > > > > > How am I supposed to get the .data without of_match_node then? > > > What's more hackish in using .data field for specific data for each > > > compatible than in using a random ID in .data and switching on it? The > > > result is exactly the same, the switching case being more verbose and > > > adding complexity to something that can be done in a straightforward manner. > > > > I've already agreed that your implementation isn't terrible, but I'd > > still like to remain strict on the rules. > > > > Better still, can you can dynamically test which platform you're on, > > via a version register or similar? > > > > Failing that, see how everyone else does it: > > > > `git grep "\.data" -- drivers/mfd/` > > Just to make sure, you prefer something like > > static struct my_struct data = { > }; > > static struct my_struct data2 = { > }; > > struct of_device_id matches[] = { > { compatible = "...", data = <ID> }, > { compatible = "...", data = <ID2> }, > }; > > of_id = of_match_device (dev, matches); > switch (of_id->data) { > case <ID>: > function(data); > case <ID2>: > function(data2); > }; > > over > > static struct my_struct data = { > }; > > static struct my_struct data2 = { > }; > > struct of_device_id matches[] = { > { compatible = "...", data = data }, > { compatible = "...", data = data2 }, > }; > > of_id = of_match_device (dev, matches); > function(of_id->data); > > ? > > This is the *only* time this is going to be used in that driver. I can > understand the need for a version if you need to apply quirks in > several functions, but here it clearly looks suboptimal. > > And we are indeed using this construct in the AXP MFD, and it just > doesn't scale either and become quite difficult to maintain when you > have a significant number of variants, and then you have to patch > *all* the switch instances to get something done. static struct my_struct data = { }; static struct my_struct data2 = { }; struct of_device_id matches[] = { { compatible = "...", data = <ID> }, { compatible = "...", data = <ID2> }, }; int probe() { struct mfd_cell *cell; of_id = of_match_device (dev, matches); switch (of_id->data) { case <ID>: cell = data; case <ID2>: cell = data2; }; mfd_add_devices(..., cell, ...) }; It's an extra few lines, but worth it to unbind MFD from DT. Is there really no way to obtain this information dynamically? -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
On 12/09/2016 15:56, Lee Jones wrote: > On Mon, 12 Sep 2016, Quentin Schulz wrote: >> On 12/09/2016 11:59, Lee Jones wrote: >>> On Mon, 12 Sep 2016, Quentin Schulz wrote: >>> >>>> On 12/09/2016 11:18, Lee Jones wrote: >>>>> On Thu, 08 Sep 2016, Quentin Schulz wrote: >>>>> [...] >>>>>> + >>>>>> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); >>>>> >>>>> Place this directly under the table. >>>>> >>>>>> +static struct platform_driver sun4i_gpadc_mfd_driver = { >>>>>> + .driver = { >>>>>> + .name = "sun4i-adc-mfd", >>>>>> + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), >>>>>> + }, >>>>>> + .probe = sun4i_gpadc_mfd_probe, >>>>> >>>>> No .remove? >>>>> >>>> >>>> No, everything in probe is handled with devm functions. >>> >>> Don't you need to undo the register write you did? >>> >> >> The regmap_write I use is there to disable all interrupts on hardware >> side before the irq_chip handles all interrupts by itself. The >> interrupts are not used in the MFD driver. >> >> Thus, I chose to disable the hardware interrupts in the remove function >> of drivers using the interrupts (only the IIO yet but the touchscreen >> driver later also which will be using a third interrupt). When the MFD >> driver is removed, the MFD cells will all be removed, thus calling their >> own remove functions, thus disabling hardware interrupts used in each >> driver. So the hardware interrupts disabling would be called twice. > > This does send some little alarm bells ringing. I'd normally expect > the .remove function to undo everything you did in .probe. So, if you > are disabling the IRQs from within the leaf drivers, shouldn't you be > initialising them in the leaf driver's respective .probes? > I use the regmap_write in the MFD driver's probe to disable all interrupts before requesting irq_chip to guarantee the interrupts are in a known state, being disabled. It is to insure no interrupt will occur unwittingly before we want the leaf drivers to handle them. The disabling of irqs in the remove is handled rather by devm_regmap_del_irq_chip than by an explicit regmap_write in the driver's removal function. It performs the exact same thing. I always use devm functions for requesting either an irq_chip or the irqs themselves. In that case, when the device is removed, the irqs are freed on leaf drivers' (where the irqs are requested) removal while the removal of irq_chip in the MFD driver will also free all irqs mapped to this irq_chip thanks to devm_regmap_del_irq_chip. Therefore, the interrupts are disabled by devm functions. The regmap_update_bits in probe and removal of the ADC driver to disable irqs are actually redundant because the devm functions already handle the irqs disabling. Thanks, Quentin
On Tue, 13 Sep 2016, Quentin Schulz wrote: > On 12/09/2016 15:56, Lee Jones wrote: > > On Mon, 12 Sep 2016, Quentin Schulz wrote: > >> On 12/09/2016 11:59, Lee Jones wrote: > >>> On Mon, 12 Sep 2016, Quentin Schulz wrote: > >>> > >>>> On 12/09/2016 11:18, Lee Jones wrote: > >>>>> On Thu, 08 Sep 2016, Quentin Schulz wrote: > >>>>> [...] > >>>>>> + > >>>>>> +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); > >>>>> > >>>>> Place this directly under the table. > >>>>> > >>>>>> +static struct platform_driver sun4i_gpadc_mfd_driver = { > >>>>>> + .driver = { > >>>>>> + .name = "sun4i-adc-mfd", > >>>>>> + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), > >>>>>> + }, > >>>>>> + .probe = sun4i_gpadc_mfd_probe, > >>>>> > >>>>> No .remove? > >>>>> > >>>> > >>>> No, everything in probe is handled with devm functions. > >>> > >>> Don't you need to undo the register write you did? > >>> > >> > >> The regmap_write I use is there to disable all interrupts on hardware > >> side before the irq_chip handles all interrupts by itself. The > >> interrupts are not used in the MFD driver. > >> > >> Thus, I chose to disable the hardware interrupts in the remove function > >> of drivers using the interrupts (only the IIO yet but the touchscreen > >> driver later also which will be using a third interrupt). When the MFD > >> driver is removed, the MFD cells will all be removed, thus calling their > >> own remove functions, thus disabling hardware interrupts used in each > >> driver. So the hardware interrupts disabling would be called twice. > > > > This does send some little alarm bells ringing. I'd normally expect > > the .remove function to undo everything you did in .probe. So, if you > > are disabling the IRQs from within the leaf drivers, shouldn't you be > > initialising them in the leaf driver's respective .probes? > > > > I use the regmap_write in the MFD driver's probe to disable all > interrupts before requesting irq_chip to guarantee the interrupts are in > a known state, being disabled. It is to insure no interrupt will occur > unwittingly before we want the leaf drivers to handle them. > > The disabling of irqs in the remove is handled rather by > devm_regmap_del_irq_chip than by an explicit regmap_write in the > driver's removal function. It performs the exact same thing. > > I always use devm functions for requesting either an irq_chip or the > irqs themselves. In that case, when the device is removed, the irqs are > freed on leaf drivers' (where the irqs are requested) removal while the > removal of irq_chip in the MFD driver will also free all irqs mapped to > this irq_chip thanks to devm_regmap_del_irq_chip. Therefore, the > interrupts are disabled by devm functions. > > The regmap_update_bits in probe and removal of the ADC driver to disable > irqs are actually redundant because the devm functions already handle > the irqs disabling. Okay. So long as you've thought about it, I'm happy. -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index 1bcf601..95b3c3e 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -29,6 +29,21 @@ config MFD_ACT8945A linear regulators, along with a complete ActivePath battery charger. +config MFD_SUN4I_GPADC + tristate "Allwinner sunxi platforms' GPADC MFD driver" + select MFD_CORE + select REGMAP_MMIO + depends on ARCH_SUNXI || COMPILE_TEST + help + Select this to get support for Allwinner SoCs (A10, A13 and A31) ADC. + This driver will only map the hardware interrupt and registers, you + have to select individual drivers based on this MFD to be able to use + the ADC or the thermal sensor. This will try to probe the ADC driver + sun4i-gpadc-iio and the hwmon driver iio_hwmon. + + To compile this driver as a module, choose M here: the module will be + called sun4i-gpadc-mfd. + config MFD_AS3711 bool "AMS AS3711" select MFD_CORE diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index 42a66e1..3b964d7 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -205,3 +205,5 @@ intel-soc-pmic-objs := intel_soc_pmic_core.o intel_soc_pmic_crc.o intel-soc-pmic-$(CONFIG_INTEL_PMC_IPC) += intel_soc_pmic_bxtwc.o obj-$(CONFIG_INTEL_SOC_PMIC) += intel-soc-pmic.o obj-$(CONFIG_MFD_MT6397) += mt6397-core.o + +obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc-mfd.o diff --git a/drivers/mfd/sun4i-gpadc-mfd.c b/drivers/mfd/sun4i-gpadc-mfd.c new file mode 100644 index 0000000..b499545 --- /dev/null +++ b/drivers/mfd/sun4i-gpadc-mfd.c @@ -0,0 +1,174 @@ +/* ADC MFD core driver for sunxi platforms + * + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + */ + +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/mfd/core.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/of_irq.h> +#include <linux/regmap.h> + +#include <linux/mfd/sun4i-gpadc-mfd.h> + +static struct resource adc_resources[] = { + { + .name = "FIFO_DATA_PENDING", + .start = SUN4I_GPADC_IRQ_FIFO_DATA, + .end = SUN4I_GPADC_IRQ_FIFO_DATA, + .flags = IORESOURCE_IRQ, + }, { + .name = "TEMP_DATA_PENDING", + .start = SUN4I_GPADC_IRQ_TEMP_DATA, + .end = SUN4I_GPADC_IRQ_TEMP_DATA, + .flags = IORESOURCE_IRQ, + }, +}; + +static const struct regmap_irq sun4i_gpadc_mfd_regmap_irq[] = { + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_FIFO_DATA, 0, + SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN), + REGMAP_IRQ_REG(SUN4I_GPADC_IRQ_TEMP_DATA, 0, + SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN), +}; + +static const struct regmap_irq_chip sun4i_gpadc_mfd_regmap_irq_chip = { + .name = "sun4i_gpadc_mfd_irq_chip", + .status_base = SUN4I_GPADC_INT_FIFOS, + .ack_base = SUN4I_GPADC_INT_FIFOS, + .mask_base = SUN4I_GPADC_INT_FIFOC, + .init_ack_masked = true, + .mask_invert = true, + .irqs = sun4i_gpadc_mfd_regmap_irq, + .num_irqs = ARRAY_SIZE(sun4i_gpadc_mfd_regmap_irq), + .num_regs = 1, +}; + +static struct mfd_cell sun4i_gpadc_mfd_cells[] = { + { + .name = "sun4i-a10-gpadc-iio", + .resources = adc_resources, + .num_resources = ARRAY_SIZE(adc_resources), + }, { + .name = "iio_hwmon", + } +}; + +static struct mfd_cell sun5i_gpadc_mfd_cells[] = { + { + .name = "sun5i-a13-gpadc-iio", + .resources = adc_resources, + .num_resources = ARRAY_SIZE(adc_resources), + }, { + .name = "iio_hwmon", + }, +}; + +static struct mfd_cell sun6i_gpadc_mfd_cells[] = { + { + .name = "sun6i-a31-gpadc-iio", + .resources = adc_resources, + .num_resources = ARRAY_SIZE(adc_resources), + }, { + .name = "iio_hwmon", + }, +}; + +static const struct regmap_config sun4i_gpadc_mfd_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, + .fast_io = true, +}; + +static const struct of_device_id sun4i_gpadc_mfd_of_match[] = { + { + .compatible = "allwinner,sun4i-a10-ts", + .data = &sun4i_gpadc_mfd_cells, + }, { + .compatible = "allwinner,sun5i-a13-ts", + .data = &sun5i_gpadc_mfd_cells, + }, { + .compatible = "allwinner,sun6i-a31-ts", + .data = &sun6i_gpadc_mfd_cells, + }, { /* sentinel */ } +}; + +static int sun4i_gpadc_mfd_probe(struct platform_device *pdev) +{ + struct sun4i_gpadc_mfd_dev *mfd_dev; + struct resource *mem; + const struct of_device_id *of_id; + const struct mfd_cell *mfd_cells; + unsigned int irq; + int ret; + + of_id = of_match_node(sun4i_gpadc_mfd_of_match, pdev->dev.of_node); + if (!of_id) + return -EINVAL; + + mfd_dev = devm_kzalloc(&pdev->dev, sizeof(*mfd_dev), GFP_KERNEL); + if (!mfd_dev) + return -ENOMEM; + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem); + if (IS_ERR(mfd_dev->regs)) + return PTR_ERR(mfd_dev->regs); + + mfd_dev->dev = &pdev->dev; + dev_set_drvdata(mfd_dev->dev, mfd_dev); + + mfd_dev->regmap = devm_regmap_init_mmio(mfd_dev->dev, mfd_dev->regs, + &sun4i_gpadc_mfd_regmap_config); + if (IS_ERR(mfd_dev->regmap)) { + ret = PTR_ERR(mfd_dev->regmap); + dev_err(&pdev->dev, "failed to init regmap: %d\n", ret); + return ret; + } + + /* Disable all interrupts */ + regmap_write(mfd_dev->regmap, SUN4I_GPADC_INT_FIFOC, 0); + + irq = platform_get_irq(pdev, 0); + ret = devm_regmap_add_irq_chip(&pdev->dev, mfd_dev->regmap, irq, + IRQF_ONESHOT, 0, + &sun4i_gpadc_mfd_regmap_irq_chip, + &mfd_dev->regmap_irqc); + if (ret) { + dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret); + return ret; + } + + mfd_cells = of_id->data; + ret = devm_mfd_add_devices(mfd_dev->dev, 0, mfd_cells, 2, NULL, 0, + NULL); + if (ret) { + dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret); + return ret; + } + + return 0; +} + +MODULE_DEVICE_TABLE(of, sun4i_gpadc_mfd_of_match); + +static struct platform_driver sun4i_gpadc_mfd_driver = { + .driver = { + .name = "sun4i-adc-mfd", + .of_match_table = of_match_ptr(sun4i_gpadc_mfd_of_match), + }, + .probe = sun4i_gpadc_mfd_probe, +}; + +module_platform_driver(sun4i_gpadc_mfd_driver); + +MODULE_DESCRIPTION("Allwinner sunxi platforms' GPADC MFD core driver"); +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/mfd/sun4i-gpadc-mfd.h b/include/linux/mfd/sun4i-gpadc-mfd.h new file mode 100644 index 0000000..5cc7863 --- /dev/null +++ b/include/linux/mfd/sun4i-gpadc-mfd.h @@ -0,0 +1,94 @@ +/* Header of ADC MFD core driver for sunxi platforms + * + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons.mfd> + * + * This program is free software; you can redistribute it and/or modify it under + * the terms of the GNU General Public License version 2 as published by the + * Free Software Foundation. + */ + +#ifndef __SUN4I_GPADC_MFD__H__ +#define __SUN4I_GPADC_MFD__H__ + +#define SUN4I_GPADC_CTRL0 0x00 + +#define SUN4I_GPADC_CTRL0_ADC_FIRST_DLY(x) ((GENMASK(7, 0) & (x)) << 24) +#define SUN4I_GPADC_CTRL0_ADC_FIRST_DLY_MODE BIT(23) +#define SUN4I_GPADC_CTRL0_ADC_CLK_SELECT BIT(22) +#define SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(x) ((GENMASK(1, 0) & (x)) << 20) +#define SUN4I_GPADC_CTRL0_FS_DIV(x) ((GENMASK(3, 0) & (x)) << 16) +#define SUN4I_GPADC_CTRL0_T_ACQ(x) (GENMASK(15, 0) & (x)) + +#define SUN4I_GPADC_CTRL1 0x04 + +#define SUN4I_GPADC_CTRL1_STYLUS_UP_DEBOUNCE(x) ((GENMASK(7, 0) & (x)) << 12) +#define SUN4I_GPADC_CTRL1_STYLUS_UP_DEBOUNCE_EN BIT(9) +#define SUN4I_GPADC_CTRL1_TOUCH_PAN_CALI_EN BIT(6) +#define SUN4I_GPADC_CTRL1_TP_DUAL_EN BIT(5) +#define SUN4I_GPADC_CTRL1_TP_MODE_EN BIT(4) +#define SUN4I_GPADC_CTRL1_TP_ADC_SELECT BIT(3) +#define SUN4I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(2, 0) & (x)) + +/* TP_CTRL1 bits for sun6i SOCs */ +#define SUN6I_GPADC_CTRL1_TOUCH_PAN_CALI_EN BIT(7) +#define SUN6I_GPADC_CTRL1_TP_DUAL_EN BIT(6) +#define SUN6I_GPADC_CTRL1_TP_MODE_EN BIT(5) +#define SUN6I_GPADC_CTRL1_TP_ADC_SELECT BIT(4) +#define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x) (GENMASK(3, 0) & BIT(x)) + +#define SUN4I_GPADC_CTRL2 0x08 + +#define SUN4I_GPADC_CTRL2_TP_SENSITIVE_ADJUST(x) ((GENMASK(3, 0) & (x)) << 28) +#define SUN4I_GPADC_CTRL2_TP_MODE_SELECT(x) ((GENMASK(1, 0) & (x)) << 26) +#define SUN4I_GPADC_CTRL2_PRE_MEA_EN BIT(24) +#define SUN4I_GPADC_CTRL2_PRE_MEA_THRE_CNT(x) (GENMASK(23, 0) & (x)) + +#define SUN4I_GPADC_CTRL3 0x0c + +#define SUN4I_GPADC_CTRL3_FILTER_EN BIT(2) +#define SUN4I_GPADC_CTRL3_FILTER_TYPE(x) (GENMASK(1, 0) & (x)) + +#define SUN4I_GPADC_TPR 0x18 + +#define SUN4I_GPADC_TPR_TEMP_ENABLE BIT(16) +#define SUN4I_GPADC_TPR_TEMP_PERIOD(x) (GENMASK(15, 0) & (x)) + +#define SUN4I_GPADC_INT_FIFOC 0x10 + +#define SUN4I_GPADC_INT_FIFOC_TEMP_IRQ_EN BIT(18) +#define SUN4I_GPADC_INT_FIFOC_TP_OVERRUN_IRQ_EN BIT(17) +#define SUN4I_GPADC_INT_FIFOC_TP_DATA_IRQ_EN BIT(16) +#define SUN4I_GPADC_INT_FIFOC_TP_DATA_XY_CHANGE BIT(13) +#define SUN4I_GPADC_INT_FIFOC_TP_FIFO_TRIG_LEVEL(x) ((GENMASK(4, 0) & (x)) << 8) +#define SUN4I_GPADC_INT_FIFOC_TP_DATA_DRQ_EN BIT(7) +#define SUN4I_GPADC_INT_FIFOC_TP_FIFO_FLUSH BIT(4) +#define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN BIT(1) +#define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN BIT(0) + +#define SUN4I_GPADC_INT_FIFOS 0x14 + +#define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING BIT(18) +#define SUN4I_GPADC_INT_FIFOS_FIFO_OVERRUN_PENDING BIT(17) +#define SUN4I_GPADC_INT_FIFOS_FIFO_DATA_PENDING BIT(16) +#define SUN4I_GPADC_INT_FIFOS_TP_IDLE_FLG BIT(2) +#define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING BIT(1) +#define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING BIT(0) + +#define SUN4I_GPADC_CDAT 0x1c +#define SUN4I_GPADC_TEMP_DATA 0x20 +#define SUN4I_GPADC_DATA 0x24 + +#define SUN4I_GPADC_IRQ_FIFO_DATA 0 +#define SUN4I_GPADC_IRQ_TEMP_DATA 1 + +/* 10s delay before suspending the IP */ +#define SUN4I_GPADC_AUTOSUSPEND_DELAY 10000 + +struct sun4i_gpadc_mfd_dev { + struct device *dev; + struct regmap *regmap; + struct regmap_irq_chip_data *regmap_irqc; + void __iomem *regs; +}; + +#endif
The Allwinner SoCs all have an ADC that can also act as a touchscreen controller and a thermal sensor. For now, only the ADC and the thermal sensor drivers are probed by the MFD, the touchscreen controller support will be added later. Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com> --- v5: - correct mail address, v4: - rename files and variables from sunxi* to sun4i*, - rename defines from SUNXI_* to SUN4I_* or SUN6I_*, - remove TP in defines name, - rename SUNXI_IRQ_* to SUN4I_GPADC_IRQ_* for consistency, - use devm functions for regmap_add_irq_chip and mfd_add_devices, - remove remove functions (now empty thanks to devm functions), v3: - use defines in regmap_irq instead of hard coded BITs, - use of_device_id data field to chose which MFD cells to add considering the compatible responsible of the MFD probe, - remove useless initializations, - disable all interrupts before adding them to regmap_irqchip, - add goto error label in probe, - correct wrapping in header license, - move defines from IIO driver to header, - use GENMASK to limit the size of the variable passed to a macro, - prefix register BIT defines with the name of the register, - reorder defines, v2: - add license headers, - reorder alphabetically includes, - add SUNXI_GPADC_ prefixes for defines, drivers/mfd/Kconfig | 15 ++++ drivers/mfd/Makefile | 2 + drivers/mfd/sun4i-gpadc-mfd.c | 174 ++++++++++++++++++++++++++++++++++++ include/linux/mfd/sun4i-gpadc-mfd.h | 94 +++++++++++++++++++ 4 files changed, 285 insertions(+) create mode 100644 drivers/mfd/sun4i-gpadc-mfd.c create mode 100644 include/linux/mfd/sun4i-gpadc-mfd.h -- 2.5.0