diff mbox

[v2,3/4] mfd: add support for Allwinner SoCs ADC

Message ID 1468576754-3273-4-git-send-email-quentin.schulz@free-electrons.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Quentin Schulz July 15, 2016, 9:59 a.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 <quentin.schulz@free-electrons.com>
---

v2:
 - add license headers,
 - reorder alphabetically includes,
 - add SUNXI_GPADC_ prefixes for defines,

 drivers/mfd/Kconfig                 |  14 +++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
 create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

Comments

Maxime Ripard July 18, 2016, 1:02 p.m. UTC | #1 
On Fri, Jul 15, 2016 at 11:59:13AM +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 <quentin.schulz@free-electrons.com>
> ---
> 
> v2:
>  - add license headers,
>  - reorder alphabetically includes,
>  - add SUNXI_GPADC_ prefixes for defines,
> 
>  drivers/mfd/Kconfig                 |  14 +++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..67b55d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
>  	  by the probe function of this MFD driver according to a device tree
>  	  property.
>  
> +config MFD_SUNXI_ADC
> +	tristate "ADC MFD core driver for sunxi platforms"
> +	select MFD_CORE
> +	select REGMAP_MMIO

It should also depends on the architectures supported (and probably 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
> +	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunxi-gpadc-mfd.
> +
>  config MFD_ATMEL_HLCDC
>  	tristate "Atmel HLCDC (High-end LCD Controller)"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..dcf43cd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> +obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
> +
>  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
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..f0005a6
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,197 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * 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/sunxi-gpadc-mfd.h>
> +
> +#define SUNXI_IRQ_FIFO_DATA	0
> +#define SUNXI_IRQ_TEMP_DATA	1
> +
> +static struct resource adc_resources[] = {
> +	{
> +		.name	= "FIFO_DATA_PENDING",
> +		.start	= SUNXI_IRQ_FIFO_DATA,
> +		.end	= SUNXI_IRQ_FIFO_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	}, {
> +		.name	= "TEMP_DATA_PENDING",
> +		.start	= SUNXI_IRQ_TEMP_DATA,
> +		.end	= SUNXI_IRQ_TEMP_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> +	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> +	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> +	.name = "sunxi_gpadc_mfd_irq_chip",
> +	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.mask_base = SUNXI_GPADC_TP_INT_FIFOC,
> +	.init_ack_masked = true,
> +	.mask_invert = true,
> +	.irqs = sunxi_gpadc_mfd_regmap_irq,
> +	.num_irqs = ARRAY_SIZE(sunxi_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 sunxi_gpadc_mfd_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
> +	struct resource *mem = NULL;
> +	unsigned int irq;
> +	int ret;
> +
> +	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
> +					   sizeof(*sunxi_gpadc_mfd_dev),
> +					   GFP_KERNEL);
> +	if (!sunxi_gpadc_mfd_dev)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
> +		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
> +
> +	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
> +	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
> +
> +	sunxi_gpadc_mfd_dev->regmap =
> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
> +				      sunxi_gpadc_mfd_dev->regs,
> +				      &sunxi_gpadc_mfd_regmap_config);

This is usually on a single line (even if it exceeds 80 chars). Or
maybe you can use a shorter variable name (like dev, or mfd).

> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
> +				  IRQF_ONESHOT, 0,
> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}

You should probably make sure that you clear all the interrupts before
enabling them.

> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "allwinner,sun4i-a10-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun4i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun5i-a13-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun5i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun6i-a31-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun6i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
> +				      0, NULL);

This huge if / else can be removed by putting those structures in the
data pointer of of_device_id.

> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "successfully loaded\n");
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	unsigned int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	mfd_remove_devices(&pdev->dev);
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
> +	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-ts" },
> +	{ .compatible = "allwinner,sun5i-a13-ts" },
> +	{ .compatible = "allwinner,sun6i-a31-ts" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
> +
> +static struct platform_driver sunxi_gpadc_mfd_driver = {
> +	.driver = {
> +		.name = "sunxi-adc-mfd",
> +		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
> +	},
> +	.probe = sunxi_gpadc_mfd_probe,
> +	.remove = sunxi_gpadc_mfd_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_mfd_driver);
> +
> +MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
> new file mode 100644
> index 0000000..7155845
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,23 @@
> +/* Header of ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * 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 __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define SUNXI_GPADC_TP_INT_FIFOC            0x10
> +#define SUNXI_GPADC_TP_INT_FIFOS            0x14

Why do you declare only these two registers there?

> +
> +struct sunxi_gpadc_mfd_dev {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +	void __iomem			*regs;
> +};
> +
> +#endif
> -- 
> 2.5.0

Thanks,
Maxime
Jonathan Cameron July 18, 2016, 1:25 p.m. UTC | #2 
On 15/07/16 10:59, 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>
Hmm. Previous patch includes the header this one creates.  Ordering issue?
The depends kind of prevents build failures by ensuring that can't be built
until this one is in place, but it is certainly an ugly way to do it.

Few little bits innline.
> ---
> 
> v2:
>  - add license headers,
>  - reorder alphabetically includes,
>  - add SUNXI_GPADC_ prefixes for defines,
> 
>  drivers/mfd/Kconfig                 |  14 +++
>  drivers/mfd/Makefile                |   2 +
>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 1bcf601..67b55d0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -82,6 +82,20 @@ config MFD_ATMEL_FLEXCOM
>  	  by the probe function of this MFD driver according to a device tree
>  	  property.
>  
> +config MFD_SUNXI_ADC
> +	tristate "ADC MFD core driver for sunxi platforms"
> +	select MFD_CORE
> +	select REGMAP_MMIO
> +	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
> +	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sunxi-gpadc-mfd.
> +
>  config MFD_ATMEL_HLCDC
>  	tristate "Atmel HLCDC (High-end LCD Controller)"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 42a66e1..dcf43cd 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -201,6 +201,8 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
>  obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
>  
> +obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
> +
>  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
> diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
> new file mode 100644
> index 0000000..f0005a6
> --- /dev/null
> +++ b/drivers/mfd/sunxi-gpadc-mfd.c
> @@ -0,0 +1,197 @@
> +/* ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * 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/sunxi-gpadc-mfd.h>
> +
> +#define SUNXI_IRQ_FIFO_DATA	0
> +#define SUNXI_IRQ_TEMP_DATA	1
> +
> +static struct resource adc_resources[] = {
> +	{
> +		.name	= "FIFO_DATA_PENDING",
> +		.start	= SUNXI_IRQ_FIFO_DATA,
> +		.end	= SUNXI_IRQ_FIFO_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	}, {
> +		.name	= "TEMP_DATA_PENDING",
> +		.start	= SUNXI_IRQ_TEMP_DATA,
> +		.end	= SUNXI_IRQ_TEMP_DATA,
> +		.flags	= IORESOURCE_IRQ,
> +	},
> +};
> +
> +static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
> +	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
> +	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
> +};
> +
> +static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
> +	.name = "sunxi_gpadc_mfd_irq_chip",
> +	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
> +	.mask_base = SUNXI_GPADC_TP_INT_FIFOC,
> +	.init_ack_masked = true,
> +	.mask_invert = true,
> +	.irqs = sunxi_gpadc_mfd_regmap_irq,
> +	.num_irqs = ARRAY_SIZE(sunxi_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",
I still really dislike using this to force the probe of that driver but
kind of up to the hwmon / mfd guys on this.

I don't have any better suggestions though..
> +	},
> +};
> +
> +static const struct regmap_config sunxi_gpadc_mfd_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.fast_io = true,
> +};
> +
> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
> +	struct resource *mem = NULL;
Neither of the above assignments is necessary as both will be explicitly
assigned before they are otherwise used.
> +	unsigned int irq;
> +	int ret;
> +
> +	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
> +					   sizeof(*sunxi_gpadc_mfd_dev),
> +					   GFP_KERNEL);
> +	if (!sunxi_gpadc_mfd_dev)
> +		return -ENOMEM;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
> +		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
> +
> +	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
> +	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
> +
> +	sunxi_gpadc_mfd_dev->regmap =
> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
> +				      sunxi_gpadc_mfd_dev->regs,
> +				      &sunxi_gpadc_mfd_regmap_config);
> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
> +		return ret;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
> +				  IRQF_ONESHOT, 0,
> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (of_device_is_compatible(pdev->dev.of_node,
> +				    "allwinner,sun4i-a10-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun4i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun5i-a13-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun5i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +	else if (of_device_is_compatible(pdev->dev.of_node,
> +					 "allwinner,sun6i-a31-ts"))
> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
> +				      sun6i_gpadc_mfd_cells,
> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
> +				      0, NULL);
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +		return ret;
> +	}
> +
> +	dev_info(&pdev->dev, "successfully loaded\n");
Seems like noise to me, but not my subsystem :)
> +
> +	return 0;
> +}
> +
> +static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
> +{
> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
> +	unsigned int irq;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	mfd_remove_devices(&pdev->dev);
> +	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
> +	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
> +	{ .compatible = "allwinner,sun4i-a10-ts" },
> +	{ .compatible = "allwinner,sun5i-a13-ts" },
> +	{ .compatible = "allwinner,sun6i-a31-ts" },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
> +
> +static struct platform_driver sunxi_gpadc_mfd_driver = {
> +	.driver = {
> +		.name = "sunxi-adc-mfd",
> +		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
> +	},
> +	.probe = sunxi_gpadc_mfd_probe,
> +	.remove = sunxi_gpadc_mfd_remove,
> +};
> +
> +module_platform_driver(sunxi_gpadc_mfd_driver);
> +
> +MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
> +MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
> new file mode 100644
> index 0000000..7155845
> --- /dev/null
> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
> @@ -0,0 +1,23 @@
> +/* Header of ADC MFD core driver for sunxi platforms
> + *
> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
> + *
> + * 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 __SUNXI_GPADC_MFD__H__
> +#define __SUNXI_GPADC_MFD__H__
> +
> +#define SUNXI_GPADC_TP_INT_FIFOC            0x10
> +#define SUNXI_GPADC_TP_INT_FIFOS            0x14
> +
> +struct sunxi_gpadc_mfd_dev {
> +	struct device			*dev;
> +	struct regmap			*regmap;
> +	struct regmap_irq_chip_data	*regmap_irqc;
> +	void __iomem			*regs;
> +};
> +
> +#endif
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones July 19, 2016, 7:31 a.m. UTC | #3 
On Mon, 18 Jul 2016, Jonathan Cameron wrote:

> On 15/07/16 10:59, 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>
> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> The depends kind of prevents build failures by ensuring that can't be built
> until this one is in place, but it is certainly an ugly way to do it.
> 
> Few little bits innline.
> > ---
> > 
> > v2:
> >  - add license headers,
> >  - reorder alphabetically includes,
> >  - add SUNXI_GPADC_ prefixes for defines,
> > 
> >  drivers/mfd/Kconfig                 |  14 +++
> >  drivers/mfd/Makefile                |   2 +
> >  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> >  4 files changed, 236 insertions(+)
> >  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> >  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

[...]

> > +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",
> I still really dislike using this to force the probe of that driver but
> kind of up to the hwmon / mfd guys on this.

Can you at least say *why* you don't like it?

How else would it get probed?

> I don't have any better suggestions though..
> > +	},
> > +};

[...]

> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> > +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> > +		return ret;
> > +	}
> > +
> > +	dev_info(&pdev->dev, "successfully loaded\n");
> Seems like noise to me, but not my subsystem :)

Agreed, I don't allow this either.

[...]
Quentin Schulz July 19, 2016, 8:35 a.m. UTC | #4 
On 18/07/2016 15:25, Jonathan Cameron wrote:
> On 15/07/16 10:59, 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>
> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> The depends kind of prevents build failures by ensuring that can't be built
> until this one is in place, but it is certainly an ugly way to do it.
> 
> Few little bits innline.
[...]
>> +static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
>> +{
>> +	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
>> +	struct resource *mem = NULL;
> Neither of the above assignments is necessary as both will be explicitly
> assigned before they are otherwise used.

ACK.

[...]
>> +	dev_info(&pdev->dev, "successfully loaded\n");
> Seems like noise to me, but not my subsystem :)

ACK.
[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Quentin Schulz July 19, 2016, 12:04 p.m. UTC | #5 
On 18/07/2016 15:02, Maxime Ripard wrote:
> On Fri, Jul 15, 2016 at 11:59:13AM +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 <quentin.schulz@free-electrons.com>
>> ---
[...]
>> +config MFD_SUNXI_ADC
>> +	tristate "ADC MFD core driver for sunxi platforms"
>> +	select MFD_CORE
>> +	select REGMAP_MMIO
> 
> It should also depends on the architectures supported (and probably COMPILE_TEST)
> 

ACK.

[...]
>> +
>> +	sunxi_gpadc_mfd_dev->regmap =
>> +		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
>> +				      sunxi_gpadc_mfd_dev->regs,
>> +				      &sunxi_gpadc_mfd_regmap_config);
> 
> This is usually on a single line (even if it exceeds 80 chars). Or
> maybe you can use a shorter variable name (like dev, or mfd).
> 

I'll go with a shorter name.

>> +	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
>> +		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
>> +		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	irq = platform_get_irq(pdev, 0);
>> +	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
>> +				  IRQF_ONESHOT, 0,
>> +				  &sunxi_gpadc_mfd_regmap_irq_chip,
>> +				  &sunxi_gpadc_mfd_dev->regmap_irqc);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
>> +		return ret;
>> +	}
> 
> You should probably make sure that you clear all the interrupts before
> enabling them.
> 

ACK. Thanks, didn't think of that.

>> +	if (of_device_is_compatible(pdev->dev.of_node,
>> +				    "allwinner,sun4i-a10-ts"))
>> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> +				      sun4i_gpadc_mfd_cells,
>> +				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
>> +				      0, NULL);
>> +	else if (of_device_is_compatible(pdev->dev.of_node,
>> +					 "allwinner,sun5i-a13-ts"))
>> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> +				      sun5i_gpadc_mfd_cells,
>> +				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
>> +				      0, NULL);
>> +	else if (of_device_is_compatible(pdev->dev.of_node,
>> +					 "allwinner,sun6i-a31-ts"))
>> +		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
>> +				      sun6i_gpadc_mfd_cells,
>> +				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
>> +				      0, NULL);
> 
> This huge if / else can be removed by putting those structures in the
> data pointer of of_device_id.
> 

Indeed. It is what I am using for the ADC driver, don't know why I
didn't think of this for the MFD as well.

[...]
>> diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
>> new file mode 100644
>> index 0000000..7155845
>> --- /dev/null
>> +++ b/include/linux/mfd/sunxi-gpadc-mfd.h
>> @@ -0,0 +1,23 @@
>> +/* Header of ADC MFD core driver for sunxi platforms
>> + *
>> + * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
>> + *
>> + * 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 __SUNXI_GPADC_MFD__H__
>> +#define __SUNXI_GPADC_MFD__H__
>> +
>> +#define SUNXI_GPADC_TP_INT_FIFOC            0x10
>> +#define SUNXI_GPADC_TP_INT_FIFOS            0x14
> 
> Why do you declare only these two registers there?
> 

Because these are used by the MFD while the others not. Maybe it's
better to put all register and bit defines in sunxi-gpadc-mfd.h? Anyway,
just found out it would be clearer to use the defines for the interrupts
rather than directly "BIT(x)". So that makes two more defines here.
Should we put everything in sunxi-gpadc-mfd.h since the MFD is needed
for the ADC, (future) touchscreen and iio_hwmon drivers?
Jonathan Cameron July 20, 2016, 3:01 p.m. UTC | #6 
On 19/07/16 08:31, Lee Jones wrote:
> On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> 
>> On 15/07/16 10:59, 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>
>> Hmm. Previous patch includes the header this one creates.  Ordering issue?
>> The depends kind of prevents build failures by ensuring that can't be built
>> until this one is in place, but it is certainly an ugly way to do it.
>>
>> Few little bits innline.
>>> ---
>>>
>>> v2:
>>>  - add license headers,
>>>  - reorder alphabetically includes,
>>>  - add SUNXI_GPADC_ prefixes for defines,
>>>
>>>  drivers/mfd/Kconfig                 |  14 +++
>>>  drivers/mfd/Makefile                |   2 +
>>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>>>  4 files changed, 236 insertions(+)
>>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> 
> [...]
> 
>>> +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",
>> I still really dislike using this to force the probe of that driver but
>> kind of up to the hwmon / mfd guys on this.
> 
> Can you at least say *why* you don't like it?
It just feels odd to have an mfd child that isn't really dependent
on the mfd hardware itself.

Still if you are happy, mfd is your domain and my objections were
as you probably noticed not that strong - or well described!)

So I'm fine with this.
> 
> How else would it get probed?
> 
>> I don't have any better suggestions though..
>>> +	},
>>> +};
> 
> [...]
> 
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
>>> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
>>> +		return ret;
>>> +	}
>>> +
>>> +	dev_info(&pdev->dev, "successfully loaded\n");
>> Seems like noise to me, but not my subsystem :)
> 
> Agreed, I don't allow this either.
> 
> [...]
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones July 21, 2016, 12:12 p.m. UTC | #7 
On Wed, 20 Jul 2016, Jonathan Cameron wrote:

> On 19/07/16 08:31, Lee Jones wrote:
> > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > 
> >> On 15/07/16 10:59, 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>
> >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> >> The depends kind of prevents build failures by ensuring that can't be built
> >> until this one is in place, but it is certainly an ugly way to do it.
> >>
> >> Few little bits innline.
> >>> ---
> >>>
> >>> v2:
> >>>  - add license headers,
> >>>  - reorder alphabetically includes,
> >>>  - add SUNXI_GPADC_ prefixes for defines,
> >>>
> >>>  drivers/mfd/Kconfig                 |  14 +++
> >>>  drivers/mfd/Makefile                |   2 +
> >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> >>>  4 files changed, 236 insertions(+)
> >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > 
> > [...]
> > 
> >>> +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",
> >> I still really dislike using this to force the probe of that driver but
> >> kind of up to the hwmon / mfd guys on this.
> > 
> > Can you at least say *why* you don't like it?
> It just feels odd to have an mfd child that isn't really dependent
> on the mfd hardware itself.
> 
> Still if you are happy, mfd is your domain and my objections were
> as you probably noticed not that strong - or well described!)
> 
> So I'm fine with this.

I see.  So it's not actually part of the same IP/chip?

> > How else would it get probed?
> > 
> >> I don't have any better suggestions though..
> >>> +	},
> >>> +};
> > 
> > [...]
> > 
> >>> +	if (ret) {
> >>> +		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
> >>> +		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	dev_info(&pdev->dev, "successfully loaded\n");
> >> Seems like noise to me, but not my subsystem :)
> > 
> > Agreed, I don't allow this either.
> > 
> > [...]
> > 
>
Maxime Ripard July 21, 2016, 8:08 p.m. UTC | #8 
1;4205;0c
On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> 
> > On 19/07/16 08:31, Lee Jones wrote:
> > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > 
> > >> On 15/07/16 10:59, 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>
> > >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> > >> The depends kind of prevents build failures by ensuring that can't be built
> > >> until this one is in place, but it is certainly an ugly way to do it.
> > >>
> > >> Few little bits innline.
> > >>> ---
> > >>>
> > >>> v2:
> > >>>  - add license headers,
> > >>>  - reorder alphabetically includes,
> > >>>  - add SUNXI_GPADC_ prefixes for defines,
> > >>>
> > >>>  drivers/mfd/Kconfig                 |  14 +++
> > >>>  drivers/mfd/Makefile                |   2 +
> > >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> > >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> > >>>  4 files changed, 236 insertions(+)
> > >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > 
> > > [...]
> > > 
> > >>> +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",
> > >> I still really dislike using this to force the probe of that driver but
> > >> kind of up to the hwmon / mfd guys on this.
> > > 
> > > Can you at least say *why* you don't like it?
> > It just feels odd to have an mfd child that isn't really dependent
> > on the mfd hardware itself.
> > 
> > Still if you are happy, mfd is your domain and my objections were
> > as you probably noticed not that strong - or well described!)
> > 
> > So I'm fine with this.
> 
> I see.  So it's not actually part of the same IP/chip?

The chip has a temperature sensor, and we want to expose that
temperature through IIO.

But we don't really have the choice on how we probe iio-hwmon
here. The binding was already there, and we have to keep it.

Maxime
Lee Jones July 22, 2016, 1:55 p.m. UTC | #9 
On Thu, 21 Jul 2016, Maxime Ripard wrote:

> 1;4205;0c
> On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> > On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> > 
> > > On 19/07/16 08:31, Lee Jones wrote:
> > > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > > 
> > > >> On 15/07/16 10:59, 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>
> > > >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> > > >> The depends kind of prevents build failures by ensuring that can't be built
> > > >> until this one is in place, but it is certainly an ugly way to do it.
> > > >>
> > > >> Few little bits innline.
> > > >>> ---
> > > >>>
> > > >>> v2:
> > > >>>  - add license headers,
> > > >>>  - reorder alphabetically includes,
> > > >>>  - add SUNXI_GPADC_ prefixes for defines,
> > > >>>
> > > >>>  drivers/mfd/Kconfig                 |  14 +++
> > > >>>  drivers/mfd/Makefile                |   2 +
> > > >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> > > >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> > > >>>  4 files changed, 236 insertions(+)
> > > >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > > >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > > 
> > > > [...]
> > > > 
> > > >>> +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",
> > > >> I still really dislike using this to force the probe of that driver but
> > > >> kind of up to the hwmon / mfd guys on this.
> > > > 
> > > > Can you at least say *why* you don't like it?
> > > It just feels odd to have an mfd child that isn't really dependent
> > > on the mfd hardware itself.
> > > 
> > > Still if you are happy, mfd is your domain and my objections were
> > > as you probably noticed not that strong - or well described!)
> > > 
> > > So I'm fine with this.
> > 
> > I see.  So it's not actually part of the same IP/chip?
> 
> The chip has a temperature sensor, and we want to expose that
> temperature through IIO.
> 
> But we don't really have the choice on how we probe iio-hwmon
> here. The binding was already there, and we have to keep it.

What binding?
Jonathan Cameron July 23, 2016, 6:42 a.m. UTC | #10 
On 22/07/16 15:55, Lee Jones wrote:
> On Thu, 21 Jul 2016, Maxime Ripard wrote:
> 
>> 1;4205;0c
>> On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
>>> On Wed, 20 Jul 2016, Jonathan Cameron wrote:
>>>
>>>> On 19/07/16 08:31, Lee Jones wrote:
>>>>> On Mon, 18 Jul 2016, Jonathan Cameron wrote:
>>>>>
>>>>>> On 15/07/16 10:59, 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>
>>>>>> Hmm. Previous patch includes the header this one creates.  Ordering issue?
>>>>>> The depends kind of prevents build failures by ensuring that can't be built
>>>>>> until this one is in place, but it is certainly an ugly way to do it.
>>>>>>
>>>>>> Few little bits innline.
>>>>>>> ---
>>>>>>>
>>>>>>> v2:
>>>>>>>  - add license headers,
>>>>>>>  - reorder alphabetically includes,
>>>>>>>  - add SUNXI_GPADC_ prefixes for defines,
>>>>>>>
>>>>>>>  drivers/mfd/Kconfig                 |  14 +++
>>>>>>>  drivers/mfd/Makefile                |   2 +
>>>>>>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
>>>>>>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
>>>>>>>  4 files changed, 236 insertions(+)
>>>>>>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
>>>>>>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
>>>>>
>>>>> [...]
>>>>>
>>>>>>> +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",
>>>>>> I still really dislike using this to force the probe of that driver but
>>>>>> kind of up to the hwmon / mfd guys on this.
>>>>>
>>>>> Can you at least say *why* you don't like it?
>>>> It just feels odd to have an mfd child that isn't really dependent
>>>> on the mfd hardware itself.
>>>>
>>>> Still if you are happy, mfd is your domain and my objections were
>>>> as you probably noticed not that strong - or well described!)
>>>>
>>>> So I'm fine with this.
>>>
>>> I see.  So it's not actually part of the same IP/chip?
>>
>> The chip has a temperature sensor, and we want to expose that
>> temperature through IIO.
>>
>> But we don't really have the choice on how we probe iio-hwmon
>> here. The binding was already there, and we have to keep it.
> 
> What binding?
> 
The somewhat 'interesting' one for iio-hwmon I would guess is what
Maxime is referring to.  Here they are using what was effectively
the old 'board file' methods inside the driver itself.  Which is
fine and done in quite a few places - though we have some issues
with deferred probing that we can improve upon.

Anyhow, it's real hardware and as this doesn't have anything to
do with device tree there is no need to get around the objections
of lack of generality that bothers the dt maintainers :)

Lets go with it as it is. Sorry for confusing everyone!

p.s. I'm pitching a tech session for the KS at the moment
on ksummit-discuss to hit the fiddlier question of bindings
/ cross subsystem interactions if anyone is interested.
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard July 25, 2016, 9:55 a.m. UTC | #11 
On Fri, Jul 22, 2016 at 02:55:56PM +0100, Lee Jones wrote:
> On Thu, 21 Jul 2016, Maxime Ripard wrote:
> 
> > 1;4205;0c
> > On Thu, Jul 21, 2016 at 01:12:53PM +0100, Lee Jones wrote:
> > > On Wed, 20 Jul 2016, Jonathan Cameron wrote:
> > > 
> > > > On 19/07/16 08:31, Lee Jones wrote:
> > > > > On Mon, 18 Jul 2016, Jonathan Cameron wrote:
> > > > > 
> > > > >> On 15/07/16 10:59, 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>
> > > > >> Hmm. Previous patch includes the header this one creates.  Ordering issue?
> > > > >> The depends kind of prevents build failures by ensuring that can't be built
> > > > >> until this one is in place, but it is certainly an ugly way to do it.
> > > > >>
> > > > >> Few little bits innline.
> > > > >>> ---
> > > > >>>
> > > > >>> v2:
> > > > >>>  - add license headers,
> > > > >>>  - reorder alphabetically includes,
> > > > >>>  - add SUNXI_GPADC_ prefixes for defines,
> > > > >>>
> > > > >>>  drivers/mfd/Kconfig                 |  14 +++
> > > > >>>  drivers/mfd/Makefile                |   2 +
> > > > >>>  drivers/mfd/sunxi-gpadc-mfd.c       | 197 ++++++++++++++++++++++++++++++++++++
> > > > >>>  include/linux/mfd/sunxi-gpadc-mfd.h |  23 +++++
> > > > >>>  4 files changed, 236 insertions(+)
> > > > >>>  create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
> > > > >>>  create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h
> > > > > 
> > > > > [...]
> > > > > 
> > > > >>> +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",
> > > > >> I still really dislike using this to force the probe of that driver but
> > > > >> kind of up to the hwmon / mfd guys on this.
> > > > > 
> > > > > Can you at least say *why* you don't like it?
> > > > It just feels odd to have an mfd child that isn't really dependent
> > > > on the mfd hardware itself.
> > > > 
> > > > Still if you are happy, mfd is your domain and my objections were
> > > > as you probably noticed not that strong - or well described!)
> > > > 
> > > > So I'm fine with this.
> > > 
> > > I see.  So it's not actually part of the same IP/chip?
> > 
> > The chip has a temperature sensor, and we want to expose that
> > temperature through IIO.
> > 
> > But we don't really have the choice on how we probe iio-hwmon
> > here. The binding was already there, and we have to keep it.
> 
> What binding?

The binding introduced for the original driver this serie aims to
replace:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/input/touchscreen/sun4i.txt

Maxime
diff mbox

Patch

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 1bcf601..67b55d0 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -82,6 +82,20 @@  config MFD_ATMEL_FLEXCOM
 	  by the probe function of this MFD driver according to a device tree
 	  property.
 
+config MFD_SUNXI_ADC
+	tristate "ADC MFD core driver for sunxi platforms"
+	select MFD_CORE
+	select REGMAP_MMIO
+	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
+	  sunxi-gpadc-iio and the hwmon driver iio_hwmon.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sunxi-gpadc-mfd.
+
 config MFD_ATMEL_HLCDC
 	tristate "Atmel HLCDC (High-end LCD Controller)"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 42a66e1..dcf43cd 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -201,6 +201,8 @@  obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 obj-$(CONFIG_MFD_RT5033)	+= rt5033.o
 obj-$(CONFIG_MFD_SKY81452)	+= sky81452.o
 
+obj-$(CONFIG_MFD_SUNXI_ADC)	+= sunxi-gpadc-mfd.o
+
 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
diff --git a/drivers/mfd/sunxi-gpadc-mfd.c b/drivers/mfd/sunxi-gpadc-mfd.c
new file mode 100644
index 0000000..f0005a6
--- /dev/null
+++ b/drivers/mfd/sunxi-gpadc-mfd.c
@@ -0,0 +1,197 @@ 
+/* ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * 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/sunxi-gpadc-mfd.h>
+
+#define SUNXI_IRQ_FIFO_DATA	0
+#define SUNXI_IRQ_TEMP_DATA	1
+
+static struct resource adc_resources[] = {
+	{
+		.name	= "FIFO_DATA_PENDING",
+		.start	= SUNXI_IRQ_FIFO_DATA,
+		.end	= SUNXI_IRQ_FIFO_DATA,
+		.flags	= IORESOURCE_IRQ,
+	}, {
+		.name	= "TEMP_DATA_PENDING",
+		.start	= SUNXI_IRQ_TEMP_DATA,
+		.end	= SUNXI_IRQ_TEMP_DATA,
+		.flags	= IORESOURCE_IRQ,
+	},
+};
+
+static const struct regmap_irq sunxi_gpadc_mfd_regmap_irq[] = {
+	REGMAP_IRQ_REG(SUNXI_IRQ_FIFO_DATA, 0, BIT(16)),
+	REGMAP_IRQ_REG(SUNXI_IRQ_TEMP_DATA, 0, BIT(18)),
+};
+
+static const struct regmap_irq_chip sunxi_gpadc_mfd_regmap_irq_chip = {
+	.name = "sunxi_gpadc_mfd_irq_chip",
+	.status_base = SUNXI_GPADC_TP_INT_FIFOS,
+	.ack_base = SUNXI_GPADC_TP_INT_FIFOS,
+	.mask_base = SUNXI_GPADC_TP_INT_FIFOC,
+	.init_ack_masked = true,
+	.mask_invert = true,
+	.irqs = sunxi_gpadc_mfd_regmap_irq,
+	.num_irqs = ARRAY_SIZE(sunxi_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 sunxi_gpadc_mfd_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+	.fast_io = true,
+};
+
+static int sunxi_gpadc_mfd_probe(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev = NULL;
+	struct resource *mem = NULL;
+	unsigned int irq;
+	int ret;
+
+	sunxi_gpadc_mfd_dev = devm_kzalloc(&pdev->dev,
+					   sizeof(*sunxi_gpadc_mfd_dev),
+					   GFP_KERNEL);
+	if (!sunxi_gpadc_mfd_dev)
+		return -ENOMEM;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	sunxi_gpadc_mfd_dev->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(sunxi_gpadc_mfd_dev->regs))
+		return PTR_ERR(sunxi_gpadc_mfd_dev->regs);
+
+	sunxi_gpadc_mfd_dev->dev = &pdev->dev;
+	dev_set_drvdata(sunxi_gpadc_mfd_dev->dev, sunxi_gpadc_mfd_dev);
+
+	sunxi_gpadc_mfd_dev->regmap =
+		devm_regmap_init_mmio(sunxi_gpadc_mfd_dev->dev,
+				      sunxi_gpadc_mfd_dev->regs,
+				      &sunxi_gpadc_mfd_regmap_config);
+	if (IS_ERR(sunxi_gpadc_mfd_dev->regmap)) {
+		ret = PTR_ERR(sunxi_gpadc_mfd_dev->regmap);
+		dev_err(&pdev->dev, "failed to init regmap: %d\n", ret);
+		return ret;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	ret = regmap_add_irq_chip(sunxi_gpadc_mfd_dev->regmap, irq,
+				  IRQF_ONESHOT, 0,
+				  &sunxi_gpadc_mfd_regmap_irq_chip,
+				  &sunxi_gpadc_mfd_dev->regmap_irqc);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add irq chip: %d\n", ret);
+		return ret;
+	}
+
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "allwinner,sun4i-a10-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun4i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun4i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+	else if (of_device_is_compatible(pdev->dev.of_node,
+					 "allwinner,sun5i-a13-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun5i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun5i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+	else if (of_device_is_compatible(pdev->dev.of_node,
+					 "allwinner,sun6i-a31-ts"))
+		ret = mfd_add_devices(sunxi_gpadc_mfd_dev->dev, 0,
+				      sun6i_gpadc_mfd_cells,
+				      ARRAY_SIZE(sun6i_gpadc_mfd_cells), NULL,
+				      0, NULL);
+
+	if (ret) {
+		dev_err(&pdev->dev, "failed to add MFD devices: %d\n", ret);
+		regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "successfully loaded\n");
+
+	return 0;
+}
+
+static int sunxi_gpadc_mfd_remove(struct platform_device *pdev)
+{
+	struct sunxi_gpadc_mfd_dev *sunxi_gpadc_mfd_dev;
+	unsigned int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	mfd_remove_devices(&pdev->dev);
+	sunxi_gpadc_mfd_dev = dev_get_drvdata(&pdev->dev);
+	regmap_del_irq_chip(irq, sunxi_gpadc_mfd_dev->regmap_irqc);
+
+	return 0;
+}
+
+static const struct of_device_id sunxi_gpadc_mfd_of_match[] = {
+	{ .compatible = "allwinner,sun4i-a10-ts" },
+	{ .compatible = "allwinner,sun5i-a13-ts" },
+	{ .compatible = "allwinner,sun6i-a31-ts" },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, sunxi_gpadc_mfd_of_match);
+
+static struct platform_driver sunxi_gpadc_mfd_driver = {
+	.driver = {
+		.name = "sunxi-adc-mfd",
+		.of_match_table = of_match_ptr(sunxi_gpadc_mfd_of_match),
+	},
+	.probe = sunxi_gpadc_mfd_probe,
+	.remove = sunxi_gpadc_mfd_remove,
+};
+
+module_platform_driver(sunxi_gpadc_mfd_driver);
+
+MODULE_DESCRIPTION("ADC MFD core driver for sunxi platforms");
+MODULE_AUTHOR("Quentin Schulz <quentin.schulz@free-electrons.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/sunxi-gpadc-mfd.h b/include/linux/mfd/sunxi-gpadc-mfd.h
new file mode 100644
index 0000000..7155845
--- /dev/null
+++ b/include/linux/mfd/sunxi-gpadc-mfd.h
@@ -0,0 +1,23 @@ 
+/* Header of ADC MFD core driver for sunxi platforms
+ *
+ * Copyright (c) 2016 Quentin Schulz <quentin.schulz@free-electrons>
+ *
+ * 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 __SUNXI_GPADC_MFD__H__
+#define __SUNXI_GPADC_MFD__H__
+
+#define SUNXI_GPADC_TP_INT_FIFOC            0x10
+#define SUNXI_GPADC_TP_INT_FIFOS            0x14
+
+struct sunxi_gpadc_mfd_dev {
+	struct device			*dev;
+	struct regmap			*regmap;
+	struct regmap_irq_chip_data	*regmap_irqc;
+	void __iomem			*regs;
+};
+
+#endif