[v5,2/3] mfd: add support for Allwinner SoCs ADC

Commit Message

Quentin Schulz Sept. 8, 2016, 2:28 p.m. UTC

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 <[e-mail]>
---

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

Comments

Maxime Ripard Sept. 9, 2016, 2:38 p.m. UTC | #1

On Thu, Sep 08, 2016 at 04:28:36PM +0200, 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 <[e-mail]>

Acked-by: Maxime Ripard <[e-mail]>

Thanks!
Maxime

Jonathan Cameron Sept. 10, 2016, 3:07 p.m. UTC | #2

On 08/09/16 15:28, 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 <[e-mail]>
I'm happy with this now.

Lee, looking for an ack from you if you want me to take this through IIO.

If you'd prefer to take this and the next patch through MFD that's
fine with me.

Acked-by: Jonathan Cameron <[e-mail]>


> ---
> 
> 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.
> +
>  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 <[e-mail]>
> + *
> + * 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 <[e-mail]>");
> +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 <[e-mail]>
> + *
> + * 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
>

Lee Jones Sept. 12, 2016, 9:18 a.m. UTC | #3

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 <[e-mail]>
> ---
> 
> 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 <[e-mail]>
> + *
> + * 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 <[e-mail]>");
> +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 <[e-mail]>
> + *
> + * 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

Quentin Schulz Sept. 12, 2016, 9:43 a.m. UTC | #4

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 <[e-mail]>
>> + *
>> + * 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

Lee Jones Sept. 12, 2016, 9:59 a.m. UTC | #5

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?

Maxime Ripard Sept. 12, 2016, 10:07 a.m. UTC | #6

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.

Maxime

Lee Jones Sept. 12, 2016, 10:49 a.m. UTC | #7

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!

Quentin Schulz Sept. 12, 2016, 10:58 a.m. UTC | #8

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

Quentin Schulz Sept. 12, 2016, 11:08 a.m. UTC | #9

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

Lee Jones Sept. 12, 2016, 1:56 p.m. UTC | #10

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 Sept. 12, 2016, 1:56 p.m. UTC | #11

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/`

Maxime Ripard Sept. 12, 2016, 2:35 p.m. UTC | #12

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.

Maxime

Lee Jones Sept. 12, 2016, 3:07 p.m. UTC | #13

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?

Quentin Schulz Sept. 13, 2016, 7:06 a.m. UTC | #14

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

Lee Jones Sept. 13, 2016, 8:21 a.m. UTC | #15

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.

Patch

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 <[e-mail]>
+ *
+ * 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 <[e-mail]>");
+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 <[e-mail]>
+ *
+ * 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