Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1444139 > unrolled thread

[PATCH v2 0/4] add support for Allwinner SoCs ADC

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] add support for Allwinner SoCs ADC Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-15 12:10 +0200
    [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-15 12:10 +0200
      Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-18 15:10 +0200
        Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-19 14:10 +0200
      Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Jonathan Cameron <jic23@kernel.org> - 2016-07-19 07:40 +0200
        Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Lee Jones <lee.jones@linaro.org> - 2016-07-19 09:40 +0200
          Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Jonathan Cameron <jic23@kernel.org> - 2016-07-20 17:10 +0200
            Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Lee Jones <lee.jones@linaro.org> - 2016-07-21 14:20 +0200
              Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-21 22:10 +0200
                Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Lee Jones <lee.jones@linaro.org> - 2016-07-22 16:00 +0200
                  Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Jonathan Cameron <jic23@kernel.org> - 2016-07-23 13:00 +0200
                  Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-25 12:00 +0200
        Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-19 10:40 +0200
    [PATCH v2 1/4] hwmon: iio_hwmon: defer probe when no channel is found Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-15 12:10 +0200
      Re: [v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found Guenter Roeck <linux@roeck-us.net> - 2016-07-16 19:10 +0200
        Re: [v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found Maxime Ripard <maxime.ripard@free-electrons.com> - 2016-07-18 12:10 +0200
          Re: [v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found Guenter Roeck <linux@roeck-us.net> - 2016-07-18 15:30 +0200
    [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-15 12:10 +0200
      Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by  iio_hwmon Guenter Roeck <linux@roeck-us.net> - 2016-07-15 16:10 +0200
        Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by  iio_hwmon Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-15 16:40 +0200
          Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by  iio_hwmon Guenter Roeck <linux@roeck-us.net> - 2016-07-16 05:00 +0200
      Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by  iio_hwmon Jonathan Cameron <jic23@kernel.org> - 2016-07-19 07:40 +0200
        Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by  iio_hwmon Quentin Schulz <quentin.schulz@free-electrons.com> - 2016-07-19 09:00 +0200
          Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by  iio_hwmon Jonathan Cameron <jic23@kernel.org> - 2016-07-20 16:50 +0200

Page 1 of 2  [1] 2  Next page →

#1444139 — [PATCH v2 0/4] add support for Allwinner SoCs ADC

The Allwinner SoCs all have an ADC that can also act as a touchscreen
controller and a thermal sensor. The first four channels can be used either
for the ADC or the touchscreen and the fifth channel is used for the
thermal sensor. We currently have a driver for the two latter functions in
drivers/input/touchscreen/sun4i-ts.c but we don't have access to the ADC
feature at all. It is meant to replace the current driver by using MFD and
subdrivers.

This adds initial support for Allwinner SoCs ADC with all features. Yet,
the touchscreen is not implemented but will be added later. To switch
between touchscreen and ADC modes, you need to poke a few bits in registers
and (de)activate an interrupt (pen-up).
An MFD is provided to let the input driver activate the pen-up interrupt
through a virtual interrupt, poke a few bits via regmap and read data from
the ADC driver while both (and iio_hwmon) are probed by the MFD.

There are slight variations between the different SoCs ADC like the address
of some registers and the scale and offset to apply to raw thermal sensor
values. These variations are handled by using different platform_device_id,
passed to the sub-drivers when they are probed by the MFD.

This also modifies iio-hwmon to allow probe deferring when no iio channel
is found. Currently when no iio channel is found, the probing of iio-hwmon
fails. This is problematic when iio-hwmon probes before the iio driver
could register iio channels to share.
It exposes labels for channels read by iio_hwmon. Until now, there were no
way to identify what the exposed channels are representing. Now, if a
channel has its extend_name field set, this value will be exposed in a
sysfs file suffixed by _label. If the field is empty, no file will be
created.

Quentin Schulz (4):
  hwmon: iio_hwmon: defer probe when no channel is found
  iio: adc: add support for Allwinner SoCs ADC
  mfd: add support for Allwinner SoCs ADC
  hwmon: iio: add label for channels read by iio_hwmon

 drivers/hwmon/iio_hwmon.c           |  82 ++++++-
 drivers/iio/adc/Kconfig             |  12 ++
 drivers/iio/adc/Makefile            |   1 +
 drivers/iio/adc/sunxi-gpadc-iio.c   | 417 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig                 |  14 ++
 drivers/mfd/Makefile                |   2 +
 drivers/mfd/sunxi-gpadc-mfd.c       | 197 +++++++++++++++++
 include/linux/mfd/sunxi-gpadc-mfd.h |  23 ++
 8 files changed, 738 insertions(+), 10 deletions(-)
 create mode 100644 drivers/iio/adc/sunxi-gpadc-iio.c
 create mode 100644 drivers/mfd/sunxi-gpadc-mfd.c
 create mode 100644 include/linux/mfd/sunxi-gpadc-mfd.h

-- 
2.5.0

[toc] | [next] | [standalone]

#1444140 — [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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
+	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
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1445472 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Multipart message — attachments visible in raw view] — view raw

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1446371 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Multipart message — attachments visible in raw view] — view raw

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?

[toc] | [prev] | [next] | [standalone]

#1446042 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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
> 

[toc] | [prev] | [next] | [standalone]

#1446194 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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.

[...]

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1447280 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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.
> 
> [...]
> 

[toc] | [prev] | [next] | [standalone]

#1447823 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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.
> > 
> > [...]
> > 
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1448111 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Multipart message — attachments visible in raw view] — view raw

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1448601 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[toc] | [prev] | [next] | [standalone]

#1448929 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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.

[toc] | [prev] | [next] | [standalone]

#1449412 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

[Multipart message — attachments visible in raw view] — view raw

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

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1446243 — Re: [PATCH v2 3/4] mfd: add support for Allwinner SoCs ADC

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.
[...]

[toc] | [prev] | [next] | [standalone]

#1444145 — [PATCH v2 1/4] hwmon: iio_hwmon: defer probe when no channel is found

iio_channel_get_all returns -ENODEV when it cannot find either phandles and
properties in the Device Tree or channels whose consumer_dev_name matches
iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
which might be probed after iio_hwmon.

It is better to defer the probe of iio_hwmon if such error is returned by
iio_channel_get_all in order to let a chance to iio drivers to expose
channels in iio_map_list.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

No modifications for this patch since we did not settled for a solution.
What should we do?

 drivers/hwmon/iio_hwmon.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index b550ba5..c0da4d9 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		name = dev->of_node->name;
 
 	channels = iio_channel_get_all(dev);
-	if (IS_ERR(channels))
+	if (IS_ERR(channels)) {
+		if (PTR_ERR(channels) == -ENODEV)
+			return -EPROBE_DEFER;
 		return PTR_ERR(channels);
+	}
 
 	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
 	if (st == NULL) {
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1444889 — Re: [v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found

On Fri, Jul 15, 2016 at 11:59:11AM +0200, Quentin Schulz wrote:
> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> properties in the Device Tree or channels whose consumer_dev_name matches
> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> which might be probed after iio_hwmon.
> 
> It is better to defer the probe of iio_hwmon if such error is returned by
> iio_channel_get_all in order to let a chance to iio drivers to expose
> channels in iio_map_list.
> 
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
> 
> No modifications for this patch since we did not settled for a solution.
> What should we do?
> 
AFAICS the only thing we can do is to replace module_platform_driver() with
its explicitly coded variant, and to use use late_initcall() instead of
module_init(). Anything else would result in endless probe deferrals if
there are no channels.

Guenter

>  drivers/hwmon/iio_hwmon.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index b550ba5..c0da4d9 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -73,8 +73,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		name = dev->of_node->name;
>  
>  	channels = iio_channel_get_all(dev);
> -	if (IS_ERR(channels))
> +	if (IS_ERR(channels)) {
> +		if (PTR_ERR(channels) == -ENODEV)
> +			return -EPROBE_DEFER;
>  		return PTR_ERR(channels);
> +	}
>  
>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>  	if (st == NULL) {

[toc] | [prev] | [next] | [standalone]

#1445359 — Re: [v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found

[Multipart message — attachments visible in raw view] — view raw

Hi Guenter,

On Sat, Jul 16, 2016 at 10:00:13AM -0700, Guenter Roeck wrote:
> On Fri, Jul 15, 2016 at 11:59:11AM +0200, Quentin Schulz wrote:
> > iio_channel_get_all returns -ENODEV when it cannot find either phandles and
> > properties in the Device Tree or channels whose consumer_dev_name matches
> > iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
> > which might be probed after iio_hwmon.
> > 
> > It is better to defer the probe of iio_hwmon if such error is returned by
> > iio_channel_get_all in order to let a chance to iio drivers to expose
> > channels in iio_map_list.
> > 
> > Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> > ---
> > 
> > No modifications for this patch since we did not settled for a solution.
> > What should we do?
> > 
> AFAICS the only thing we can do is to replace module_platform_driver() with
> its explicitly coded variant, and to use use late_initcall() instead of
> module_init().

I thought this kind of changes to the driver init time were
discouraged these days?

> Anything else would result in endless probe deferrals if there are
> no channels.

Well, technically, not endless. AFAIK, the kernel only retries when a
new driver is probed, which should hopefully settle down rather
quickly.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1445479 — Re: [v2,1/4] hwmon: iio_hwmon: defer probe when no channel is found

On 07/18/2016 03:02 AM, Maxime Ripard wrote:
> Hi Guenter,
>
> On Sat, Jul 16, 2016 at 10:00:13AM -0700, Guenter Roeck wrote:
>> On Fri, Jul 15, 2016 at 11:59:11AM +0200, Quentin Schulz wrote:
>>> iio_channel_get_all returns -ENODEV when it cannot find either phandles and
>>> properties in the Device Tree or channels whose consumer_dev_name matches
>>> iio_hwmon in iio_map_list. The iio_map_list is filled in by iio drivers
>>> which might be probed after iio_hwmon.
>>>
>>> It is better to defer the probe of iio_hwmon if such error is returned by
>>> iio_channel_get_all in order to let a chance to iio drivers to expose
>>> channels in iio_map_list.
>>>
>>> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
>>> ---
>>>
>>> No modifications for this patch since we did not settled for a solution.
>>> What should we do?
>>>
>> AFAICS the only thing we can do is to replace module_platform_driver() with
>> its explicitly coded variant, and to use use late_initcall() instead of
>> module_init().
>
> I thought this kind of changes to the driver init time were
> discouraged these days?
>

Blindly converting -ENODEV to -EPROBEDEFER doesn't sound like a good idea
either.

>> Anything else would result in endless probe deferrals if there are
>> no channels.
>
> Well, technically, not endless. AFAIK, the kernel only retries when a
> new driver is probed, which should hopefully settle down rather
> quickly.
>

Plus each time a new driver is loaded for whatever reason.

Also, are you sure that leaving a device in deferred probe state doesn't
have undesirable side effects ? For example, would driver_probe_done()
ever return true ?

Guenter

[toc] | [prev] | [next] | [standalone]

#1444146 — [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

Currently, iio_hwmon only exposes values of the IIO channels it can read
but no label by channel is exposed.

This adds exposition of sysfs files containing label for IIO channels it
can read based on extended_name field of the iio_chan_spec of the channel.
If the extended_name field is empty, the sysfs file is not created by
iio_hwmon.

Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
---

patch added in v2

 drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 68 insertions(+), 9 deletions(-)

diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
index c0da4d9..28d15b2 100644
--- a/drivers/hwmon/iio_hwmon.c
+++ b/drivers/hwmon/iio_hwmon.c
@@ -16,6 +16,7 @@
 #include <linux/of.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/iio/consumer.h>
+#include <linux/iio/iio.h>
 #include <linux/iio/types.h>
 
 /**
@@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
 	return sprintf(buf, "%d\n", result);
 }
 
+static ssize_t iio_hwmon_read_label(struct device *dev,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
+	struct iio_hwmon_state *state = dev_get_drvdata(dev);
+	const char *label = state->channels[sattr->index].channel->extend_name;
+
+	if (label)
+		return sprintf(buf, "%s\n", label);
+
+	return 0;
+}
+
 static int iio_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct iio_hwmon_state *st;
-	struct sensor_device_attribute *a;
-	int ret, i;
+	struct sensor_device_attribute *a, *b;
+	int ret, i, j = 0;
 	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
 	enum iio_chan_type type;
 	struct iio_channel *channels;
@@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		st->num_channels++;
 
 	st->attrs = devm_kzalloc(dev,
-				 sizeof(*st->attrs) * (st->num_channels + 1),
+				 sizeof(*st->attrs) *
+				 (2 * st->num_channels + 1),
 				 GFP_KERNEL);
 	if (st->attrs == NULL) {
 		ret = -ENOMEM;
@@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		}
 
 		sysfs_attr_init(&a->dev_attr.attr);
+
+		b = NULL;
+		if (st->channels[i].channel->extend_name) {
+			b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
+			if (b == NULL) {
+				ret = -ENOMEM;
+				goto error_release_channels;
+			}
+
+			sysfs_attr_init(&b->dev_attr.attr);
+		}
+
 		ret = iio_get_channel_type(&st->channels[i], &type);
 		if (ret < 0)
 			goto error_release_channels;
@@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		case IIO_VOLTAGE:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "in%d_input",
-							  in_i++);
+							  in_i);
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "in%d_label",
+								  in_i);
+			in_i++;
 			break;
 		case IIO_TEMP:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "temp%d_input",
-							  temp_i++);
+							  temp_i);
+
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "temp%d_label",
+								  temp_i);
+			temp_i++;
 			break;
 		case IIO_CURRENT:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "curr%d_input",
-							  curr_i++);
+							  curr_i);
+
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "curr%d_label",
+								  curr_i);
+			curr_i++;
 			break;
 		case IIO_HUMIDITYRELATIVE:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
 							  "humidity%d_input",
-							  humidity_i++);
+							  humidity_i);
+
+			if (b)
+				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
+								  "humidity%d_label",
+								  humidity_i);
+			humidity_i++;
 			break;
 		default:
 			ret = -EINVAL;
 			goto error_release_channels;
 		}
-		if (a->dev_attr.attr.name == NULL) {
+		if (a->dev_attr.attr.name == NULL ||
+		    (b && b->dev_attr.attr.name == NULL)) {
 			ret = -ENOMEM;
 			goto error_release_channels;
 		}
 		a->dev_attr.show = iio_hwmon_read_val;
 		a->dev_attr.attr.mode = S_IRUGO;
 		a->index = i;
-		st->attrs[i] = &a->dev_attr.attr;
+		st->attrs[j++] = &a->dev_attr.attr;
+
+		if (b) {
+			b->dev_attr.show = iio_hwmon_read_label;
+			b->dev_attr.attr.mode = S_IRUGO;
+			b->index = i;
+			st->attrs[j++] = &b->dev_attr.attr;
+		}
 	}
 
 	st->attr_group.attrs = st->attrs;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1444339 — Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

On 07/15/2016 02:59 AM, Quentin Schulz wrote:
> Currently, iio_hwmon only exposes values of the IIO channels it can read
> but no label by channel is exposed.
>
> This adds exposition of sysfs files containing label for IIO channels it
> can read based on extended_name field of the iio_chan_spec of the channel.
> If the extended_name field is empty, the sysfs file is not created by
> iio_hwmon.
>
> Signed-off-by: Quentin Schulz <quentin.schulz@free-electrons.com>
> ---
>
> patch added in v2
>
>   drivers/hwmon/iio_hwmon.c | 77 +++++++++++++++++++++++++++++++++++++++++------
>   1 file changed, 68 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/iio_hwmon.c b/drivers/hwmon/iio_hwmon.c
> index c0da4d9..28d15b2 100644
> --- a/drivers/hwmon/iio_hwmon.c
> +++ b/drivers/hwmon/iio_hwmon.c
> @@ -16,6 +16,7 @@
>   #include <linux/of.h>
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/iio/consumer.h>
> +#include <linux/iio/iio.h>
>   #include <linux/iio/types.h>
>
>   /**
> @@ -57,12 +58,26 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>   	return sprintf(buf, "%d\n", result);
>   }
>
> +static ssize_t iio_hwmon_read_label(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
> +	struct iio_hwmon_state *state = dev_get_drvdata(dev);
> +	const char *label = state->channels[sattr->index].channel->extend_name;
> +
> +	if (label)
> +		return sprintf(buf, "%s\n", label);
> +
Can the name disappear on the fly, or be changed on the fly ?
Then this is unusable. We should and can only provide labels
if a name exists and is permanent. Otherwise all we do is
to confuse user space.

> +	return 0;
> +}
> +
>   static int iio_hwmon_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
>   	struct iio_hwmon_state *st;
> -	struct sensor_device_attribute *a;
> -	int ret, i;
> +	struct sensor_device_attribute *a, *b;
> +	int ret, i, j = 0;
>   	int in_i = 1, temp_i = 1, curr_i = 1, humidity_i = 1;
>   	enum iio_chan_type type;
>   	struct iio_channel *channels;
> @@ -92,7 +107,8 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		st->num_channels++;
>
>   	st->attrs = devm_kzalloc(dev,
> -				 sizeof(*st->attrs) * (st->num_channels + 1),
> +				 sizeof(*st->attrs) *
> +				 (2 * st->num_channels + 1),
>   				 GFP_KERNEL);
>   	if (st->attrs == NULL) {
>   		ret = -ENOMEM;
> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		}
>
>   		sysfs_attr_init(&a->dev_attr.attr);
> +
> +		b = NULL;
> +		if (st->channels[i].channel->extend_name) {
> +			b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
> +			if (b == NULL) {
> +				ret = -ENOMEM;
> +				goto error_release_channels;
> +			}
> +
> +			sysfs_attr_init(&b->dev_attr.attr);

Why is this initialization here and not with the rest of the initialization
of this attribute ?

> +		}
> +
>   		ret = iio_get_channel_type(&st->channels[i], &type);
>   		if (ret < 0)
>   			goto error_release_channels;
> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>   		case IIO_VOLTAGE:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "in%d_input",
> -							  in_i++);
> +							  in_i);
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "in%d_label",
> +								  in_i);
> +			in_i++;
>   			break;
>   		case IIO_TEMP:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "temp%d_input",
> -							  temp_i++);
> +							  temp_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "temp%d_label",
> +								  temp_i);
> +			temp_i++;
>   			break;
>   		case IIO_CURRENT:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "curr%d_input",
> -							  curr_i++);
> +							  curr_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "curr%d_label",
> +								  curr_i);
> +			curr_i++;
>   			break;
>   		case IIO_HUMIDITYRELATIVE:
>   			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>   							  "humidity%d_input",
> -							  humidity_i++);
> +							  humidity_i);
> +
> +			if (b)
> +				b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> +								  "humidity%d_label",
> +								  humidity_i);
> +			humidity_i++;
>   			break;
>   		default:
>   			ret = -EINVAL;
>   			goto error_release_channels;
>   		}
> -		if (a->dev_attr.attr.name == NULL) {
> +		if (a->dev_attr.attr.name == NULL ||
> +		    (b && b->dev_attr.attr.name == NULL)) {
>   			ret = -ENOMEM;
>   			goto error_release_channels;
>   		}

Just realized that we have a memory leak here. The 'name' memory is never released.

>   		a->dev_attr.show = iio_hwmon_read_val;
>   		a->dev_attr.attr.mode = S_IRUGO;
>   		a->index = i;
> -		st->attrs[i] = &a->dev_attr.attr;
> +		st->attrs[j++] = &a->dev_attr.attr;
> +
> +		if (b) {
> +			b->dev_attr.show = iio_hwmon_read_label;
> +			b->dev_attr.attr.mode = S_IRUGO;
> +			b->index = i;
> +			st->attrs[j++] = &b->dev_attr.attr;
> +		}
>   	}
>
>   	st->attr_group.attrs = st->attrs;
>

[toc] | [prev] | [next] | [standalone]

#1444347 — Re: [PATCH v2 4/4] hwmon: iio: add label for channels read by iio_hwmon

On 15/07/2016 16:03, Guenter Roeck wrote:
> On 07/15/2016 02:59 AM, Quentin Schulz wrote:
[...]
>> +static ssize_t iio_hwmon_read_label(struct device *dev,
>> +                    struct device_attribute *attr,
>> +                    char *buf)
>> +{
>> +    struct sensor_device_attribute *sattr = to_sensor_dev_attr(attr);
>> +    struct iio_hwmon_state *state = dev_get_drvdata(dev);
>> +    const char *label =
>> state->channels[sattr->index].channel->extend_name;
>> +
>> +    if (label)
>> +        return sprintf(buf, "%s\n", label);
>> +
> Can the name disappear on the fly, or be changed on the fly ?
> Then this is unusable. We should and can only provide labels
> if a name exists and is permanent. Otherwise all we do is
> to confuse user space.

It cannot, the extend_name field is const char* in the struct:
http://lxr.free-electrons.com/source/include/linux/iio/iio.h#L247

[...]
>> @@ -107,6 +123,18 @@ static int iio_hwmon_probe(struct platform_device
>> *pdev)
>>           }
>>
>>           sysfs_attr_init(&a->dev_attr.attr);
>> +
>> +        b = NULL;
>> +        if (st->channels[i].channel->extend_name) {
>> +            b = devm_kzalloc(dev, sizeof(*b), GFP_KERNEL);
>> +            if (b == NULL) {
>> +                ret = -ENOMEM;
>> +                goto error_release_channels;
>> +            }
>> +
>> +            sysfs_attr_init(&b->dev_attr.attr);
> 
> Why is this initialization here and not with the rest of the initialization
> of this attribute ?

I don't get your question. I've followed the exact same pattern as for
"a" variable's initialization.
The initialization is before the switch case because the name of the
exposed sysfs file depends on the type of the IIO channel. If I move the
initialization in the switch case, I'll have duplicated code.

>> +        }
>> +
>>           ret = iio_get_channel_type(&st->channels[i], &type);
>>           if (ret < 0)
>>               goto error_release_channels;
>> @@ -115,35 +143,66 @@ static int iio_hwmon_probe(struct
>> platform_device *pdev)
>>           case IIO_VOLTAGE:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "in%d_input",
>> -                              in_i++);
>> +                              in_i);
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "in%d_label",
>> +                                  in_i);
>> +            in_i++;
>>               break;
>>           case IIO_TEMP:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "temp%d_input",
>> -                              temp_i++);
>> +                              temp_i);
>> +
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "temp%d_label",
>> +                                  temp_i);
>> +            temp_i++;
>>               break;
>>           case IIO_CURRENT:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "curr%d_input",
>> -                              curr_i++);
>> +                              curr_i);
>> +
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "curr%d_label",
>> +                                  curr_i);
>> +            curr_i++;
>>               break;
>>           case IIO_HUMIDITYRELATIVE:
>>               a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>>                                 "humidity%d_input",
>> -                              humidity_i++);
>> +                              humidity_i);
>> +
>> +            if (b)
>> +                b->dev_attr.attr.name = kasprintf(GFP_KERNEL,
>> +                                  "humidity%d_label",
>> +                                  humidity_i);
>> +            humidity_i++;
>>               break;
>>           default:
>>               ret = -EINVAL;
>>               goto error_release_channels;
>>           }
>> -        if (a->dev_attr.attr.name == NULL) {
>> +        if (a->dev_attr.attr.name == NULL ||
>> +            (b && b->dev_attr.attr.name == NULL)) {
>>               ret = -ENOMEM;
>>               goto error_release_channels;
>>           }
> 
> Just realized that we have a memory leak here. The 'name' memory is
> never released.
> 

I don't know if we have to do something to revert the effects of
sysfs_attr_init but you sure are right that the a and b's attribute's
name is never freed. This case would be handled with devm_kasprintf, I
guess? Thanks.

>>           a->dev_attr.show = iio_hwmon_read_val;
>>           a->dev_attr.attr.mode = S_IRUGO;
>>           a->index = i;
>> -        st->attrs[i] = &a->dev_attr.attr;
>> +        st->attrs[j++] = &a->dev_attr.attr;
>> +
>> +        if (b) {
>> +            b->dev_attr.show = iio_hwmon_read_label;
>> +            b->dev_attr.attr.mode = S_IRUGO;
>> +            b->index = i;
>> +            st->attrs[j++] = &b->dev_attr.attr;
>> +        }
>>       }
>>
>>       st->attr_group.attrs = st->attrs;
>>
> 

Quentin

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web