[PATCH 0/3] mxs-lradc: Add support for current sources

[PATCH 0/3] mxs-lradc: Add support for current sources

[Harald Geyer]

Patch 1/3 changes the driver and updates the binding documentation
(I guess it is still in staging.)

Patches 2/3 and 3/3 add the devicetree nodes to imx23 and imx28 boards.
I'd like to get input whether this is actually desired. On boards where
these regulators would never be enabled this costs a few extra bytes of
RAM for allocation of the device data, because the nodes can't be easily
removed in .dts files which are including the .dtsi files. The alternative
is to add the new nodes to many .dts files, which would be a lot code
duplication.

Harald Geyer (3):
  iio: mxs-lradc: Add regulators for current sources
  ARM: dts: imx23: Provide regulators for the current sources of the
    LRADC
  ARM: dts: imx28: Provide regulators for the current sources of the
    LRADC

 .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
 arch/arm/boot/dts/imx23.dtsi                       |   8 ++
 arch/arm/boot/dts/imx28.dtsi                       |   8 ++
 drivers/iio/adc/Kconfig                            |   1 +
 drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
 5 files changed, 198 insertions(+)

-- 
2.1.4

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Harald Geyer]

The hardware has two current sources ISRC0 and ISRC1 to allow measuring
resistors without additional circuitry. This commit makes them available
as regulators.

Tested on an imx233-olinuxino board.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
The current regulator API doesn't fit this type of device very well: Typically
consumers will want to set a defined current, ie. min_uA == max_uA, but they
can't without help from configuration data, because the valid values aren't
reported by the API for current regulators. I have been thinking about
extending the API, but currently AFAIK no such consumers exist and most
users, like myself, will force the regulator to a defined value in
devicetree anyway.

 .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
 drivers/iio/adc/Kconfig                            |   1 +
 drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
 3 files changed, 182 insertions(+)

diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
index 555fb11..983952c 100644
--- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
+++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
@@ -19,6 +19,15 @@ Optional properties:
 - fsl,settling: delay between plate switch to next sample. Allowed value is
                 1 ... 2047. It counts at 2 kHz and its default is
                 10 (= 5 ms)
+- ISRC0: A node describing the regulator of internal current source 0
+- ISRC1: A node describing the regulator of internal current source 1
+
+Required properties for the ISRCx sub-nodes:
+- regulator-max-microamp: See standard regulator binding documentation.
+                          Valid values are from 0 to 300 in steps of 20.
+
+Optional properties for the ISRCx sub-nodes:
+Any standard regulator properties that apply to current regulators.
 
 Example for i.MX23 SoC:
 
@@ -31,6 +40,16 @@ Example for i.MX23 SoC:
 		fsl,ave-ctrl = <4>;
 		fsl,ave-delay = <2>;
 		fsl,settling = <10>;
+
+		isrc_lradc0: ISRC0 {
+			regulator-max-microamp = <300>;
+		};
+
+		isrc_lradc1: ISRC1 {
+			regulator-max-microamp = <120>;
+			regulator-min-microamp = <120>;
+			regulator-always-on;
+		};
 	};
 
 Example for i.MX28 SoC:
@@ -44,4 +63,14 @@ Example for i.MX28 SoC:
 		fsl,ave-ctrl = <4>;
 		fsl,ave-delay = <2>;
 		fsl,settling = <10>;
+
+		isrc_lradc0: ISRC0 {
+			regulator-max-microamp = <300>;
+		};
+
+		isrc_lradc6: ISRC1 {
+			regulator-max-microamp = <120>;
+			regulator-min-microamp = <120>;
+			regulator-always-on;
+		};
 	};
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 5937030..1968d1c 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -319,6 +319,7 @@ config MXS_LRADC
         tristate "Freescale i.MX23/i.MX28 LRADC"
         depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
         depends on INPUT
+        depends on REGULATOR
         select STMP_DEVICE
         select IIO_BUFFER
         select IIO_TRIGGERED_BUFFER
diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
index 33051b8..f22f339 100644
--- a/drivers/iio/adc/mxs-lradc.c
+++ b/drivers/iio/adc/mxs-lradc.c
@@ -40,6 +40,10 @@
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/sysfs.h>
 
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+
 #define DRIVER_NAME		"mxs-lradc"
 
 #define LRADC_MAX_DELAY_CHANS	4
@@ -261,6 +265,9 @@ struct mxs_lradc {
 	unsigned		over_sample_delay;
 	/* time in clocks to wait after the plates where switched */
 	unsigned		settling_delay;
+
+	struct regulator_desc isrc0;
+	struct regulator_desc isrc1;
 };
 
 #define	LRADC_CTRL0				0x00
@@ -305,6 +312,11 @@ struct mxs_lradc {
 #define	LRADC_CTRL2				0x20
 #define	LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET	24
 #define	LRADC_CTRL2_TEMPSENSE_PWD		BIT(15)
+#define	LRADC_CTRL2_TEMP_SENSOR_IENABLE1	BIT(9)
+#define	LRADC_CTRL2_TEMP_SENSOR_IENABLE0	BIT(8)
+#define	LRADC_CTRL2_TEMP_ISRC1_OFFSET		4
+#define	LRADC_CTRL2_TEMP_ISRC0_OFFSET		0
+#define	LRADC_CTRL2_TEMP_ISRC_MASK		0x0f
 
 #define	LRADC_STATUS				0x40
 #define	LRADC_STATUS_TOUCH_DETECT_RAW		BIT(0)
@@ -1383,6 +1395,109 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
 	.validate_scan_mask = &mxs_lradc_validate_scan_mask,
 };
 
+static int mxs_lradc_regulator_is_enabled(struct regulator_dev *dev)
+{
+	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
+	int reg = readl(lradc->base + LRADC_CTRL2);
+
+	if (dev->desc == &lradc->isrc0)
+		return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE0;
+	else if (dev->desc == &lradc->isrc1)
+		return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE1;
+
+	/* This should never happen */
+	return -ENODEV;
+}
+
+#define LRADC_REGVALUE2uA(regval, offset) \
+	(20 * ((regval >> offset) & LRADC_CTRL2_TEMP_ISRC_MASK))
+
+static int mxs_lradc_regulator_get_current_limit(struct regulator_dev *dev)
+{
+	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
+	int reg = readl(lradc->base + LRADC_CTRL2);
+
+	if (dev->desc == &lradc->isrc0)
+		return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC0_OFFSET);
+	else if (dev->desc == &lradc->isrc1)
+		return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC1_OFFSET);
+
+	/* This should never happen */
+	return -ENODEV;
+}
+
+static int mxs_lradc_regulator_enable(struct regulator_dev *dev)
+{
+	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
+
+	if (dev->desc == &lradc->isrc0)
+		mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
+				  LRADC_CTRL2);
+	else if (dev->desc == &lradc->isrc1)
+		mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
+				  LRADC_CTRL2);
+	else
+		/* This should never happen */
+		return -ENODEV;
+
+	return 0;
+}
+
+static int mxs_lradc_regulator_disable(struct regulator_dev *dev)
+{
+	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
+
+	if (dev->desc == &lradc->isrc0)
+		mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
+				    LRADC_CTRL2);
+	else if (dev->desc == &lradc->isrc1)
+		mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
+				    LRADC_CTRL2);
+	else
+		/* This should never happen */
+		return -ENODEV;
+
+	return 0;
+}
+
+static int mxs_lradc_regulator_set_current_limit(struct regulator_dev *dev,
+						 int min_uA, int max_uA)
+{
+	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
+	int offset, value;
+
+	if (dev->desc == &lradc->isrc0)
+		offset = LRADC_CTRL2_TEMP_ISRC0_OFFSET;
+	else if (dev->desc == &lradc->isrc1)
+		offset = LRADC_CTRL2_TEMP_ISRC1_OFFSET;
+	else
+		/* This should never happen */
+		return -ENODEV;
+
+	value = min_uA / 20;
+	if (min_uA % 20)
+		value++;
+	if (value * 20 > max_uA)
+		return -EINVAL;
+	if (value & ~LRADC_CTRL2_TEMP_ISRC_MASK)
+		/* This should never happen */
+		return -EPERM;
+
+	mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_ISRC_MASK << offset,
+			    LRADC_CTRL2);
+	mxs_lradc_reg_set(lradc, value << offset, LRADC_CTRL2);
+
+	return 0;
+}
+
+static struct regulator_ops mxs_lradc_regulator_current_ops = {
+	.enable = mxs_lradc_regulator_enable,
+	.is_enabled = mxs_lradc_regulator_is_enabled,
+	.disable = mxs_lradc_regulator_disable,
+	.get_current_limit = mxs_lradc_regulator_get_current_limit,
+	.set_current_limit = mxs_lradc_regulator_set_current_limit,
+};
+
 /*
  * Driver initialization
  */
@@ -1519,6 +1634,10 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
 
 	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
 		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
+
+	mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0 |
+					LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
+			    LRADC_CTRL2);
 }
 
 static const struct of_device_id mxs_lradc_dt_ids[] = {
@@ -1592,6 +1711,32 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
 	return 0;
 }
 
+static void mxs_lradc_reg_helper(struct device_node *np, const char *name,
+				 struct regulator_config *conf,
+				 struct regulator_desc *desc)
+{
+	struct regulator_dev *ret;
+
+	conf->of_node = of_get_child_by_name(np, name);
+	if (!conf->of_node)
+		return;
+
+	desc->name = name;
+	desc->owner = THIS_MODULE;
+	desc->type = REGULATOR_CURRENT;
+	desc->ops = &mxs_lradc_regulator_current_ops;
+
+	conf->init_data = of_get_regulator_init_data(conf->dev, conf->of_node,
+						     desc);
+	ret = devm_regulator_register(conf->dev, desc, conf);
+	if (IS_ERR(ret))
+		/* Just pretend the regulator isn't there */
+		dev_err(conf->dev, "Failed to register regulator %s: %ld\n",
+			desc->name, PTR_ERR(ret));
+
+	of_node_put(conf->of_node);
+}
+
 static int mxs_lradc_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *of_id =
@@ -1603,6 +1748,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 	struct mxs_lradc *lradc;
 	struct iio_dev *iio;
 	struct resource *iores;
+	struct regulator_config regconf;
 	int ret = 0, touch_ret;
 	int i, s;
 	u64 scale_uv;
@@ -1727,6 +1873,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
 		goto err_ts;
 	}
 
+	/* Setup regulator devices for current source. */
+	regconf.dev = dev;
+	regconf.driver_data = lradc;
+	mxs_lradc_reg_helper(node, "ISRC0", &regconf, &lradc->isrc0);
+	mxs_lradc_reg_helper(node, "ISRC1", &regconf, &lradc->isrc1);
+
 	return 0;
 
 err_ts:
-- 
2.1.4

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Marek Vasut]

On 04/22/2016 03:52 PM, Harald Geyer wrote:
> The hardware has two current sources ISRC0 and ISRC1 to allow measuring
> resistors without additional circuitry. This commit makes them available
> as regulators.
> 
> Tested on an imx233-olinuxino board.
> 
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> The current regulator API doesn't fit this type of device very well: Typically
> consumers will want to set a defined current, ie. min_uA == max_uA, but they
> can't without help from configuration data, because the valid values aren't
> reported by the API for current regulators. I have been thinking about
> extending the API, but currently AFAIK no such consumers exist and most
> users, like myself, will force the regulator to a defined value in
> devicetree anyway.

I am tempted to block this patch and ask you to properly split the
mxs-lradc driver into MFD with touchscreen and IIO part and only then
add the regulator bits. The lradc is becoming a katamari of ad-hoc
misplaced functionality.

>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>  drivers/iio/adc/Kconfig                            |   1 +
>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>  3 files changed, 182 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> index 555fb11..983952c 100644
> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> @@ -19,6 +19,15 @@ Optional properties:
>  - fsl,settling: delay between plate switch to next sample. Allowed value is
>                  1 ... 2047. It counts at 2 kHz and its default is
>                  10 (= 5 ms)
> +- ISRC0: A node describing the regulator of internal current source 0
> +- ISRC1: A node describing the regulator of internal current source 1
> +
> +Required properties for the ISRCx sub-nodes:
> +- regulator-max-microamp: See standard regulator binding documentation.
> +                          Valid values are from 0 to 300 in steps of 20.
> +
> +Optional properties for the ISRCx sub-nodes:
> +Any standard regulator properties that apply to current regulators.
>  
>  Example for i.MX23 SoC:
>  
> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>  		fsl,ave-ctrl = <4>;
>  		fsl,ave-delay = <2>;
>  		fsl,settling = <10>;
> +
> +		isrc_lradc0: ISRC0 {
> +			regulator-max-microamp = <300>;
> +		};
> +
> +		isrc_lradc1: ISRC1 {
> +			regulator-max-microamp = <120>;
> +			regulator-min-microamp = <120>;
> +			regulator-always-on;
> +		};
>  	};
>  
>  Example for i.MX28 SoC:
> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>  		fsl,ave-ctrl = <4>;
>  		fsl,ave-delay = <2>;
>  		fsl,settling = <10>;
> +
> +		isrc_lradc0: ISRC0 {
> +			regulator-max-microamp = <300>;
> +		};
> +
> +		isrc_lradc6: ISRC1 {
> +			regulator-max-microamp = <120>;
> +			regulator-min-microamp = <120>;
> +			regulator-always-on;
> +		};
>  	};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5937030..1968d1c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -319,6 +319,7 @@ config MXS_LRADC
>          tristate "Freescale i.MX23/i.MX28 LRADC"
>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>          depends on INPUT
> +        depends on REGULATOR
>          select STMP_DEVICE
>          select IIO_BUFFER
>          select IIO_TRIGGERED_BUFFER
> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
> index 33051b8..f22f339 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -40,6 +40,10 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/sysfs.h>
>  
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +
>  #define DRIVER_NAME		"mxs-lradc"
>  
>  #define LRADC_MAX_DELAY_CHANS	4
> @@ -261,6 +265,9 @@ struct mxs_lradc {
>  	unsigned		over_sample_delay;
>  	/* time in clocks to wait after the plates where switched */
>  	unsigned		settling_delay;
> +
> +	struct regulator_desc isrc0;
> +	struct regulator_desc isrc1;
>  };
>  
>  #define	LRADC_CTRL0				0x00
> @@ -305,6 +312,11 @@ struct mxs_lradc {
>  #define	LRADC_CTRL2				0x20
>  #define	LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET	24
>  #define	LRADC_CTRL2_TEMPSENSE_PWD		BIT(15)
> +#define	LRADC_CTRL2_TEMP_SENSOR_IENABLE1	BIT(9)
> +#define	LRADC_CTRL2_TEMP_SENSOR_IENABLE0	BIT(8)
> +#define	LRADC_CTRL2_TEMP_ISRC1_OFFSET		4
> +#define	LRADC_CTRL2_TEMP_ISRC0_OFFSET		0
> +#define	LRADC_CTRL2_TEMP_ISRC_MASK		0x0f
>  
>  #define	LRADC_STATUS				0x40
>  #define	LRADC_STATUS_TOUCH_DETECT_RAW		BIT(0)
> @@ -1383,6 +1395,109 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
>  	.validate_scan_mask = &mxs_lradc_validate_scan_mask,
>  };
>  
> +static int mxs_lradc_regulator_is_enabled(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +	int reg = readl(lradc->base + LRADC_CTRL2);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE0;
> +	else if (dev->desc == &lradc->isrc1)
> +		return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE1;
> +
> +	/* This should never happen */
> +	return -ENODEV;
> +}
> +
> +#define LRADC_REGVALUE2uA(regval, offset) \
> +	(20 * ((regval >> offset) & LRADC_CTRL2_TEMP_ISRC_MASK))
> +
> +static int mxs_lradc_regulator_get_current_limit(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +	int reg = readl(lradc->base + LRADC_CTRL2);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC0_OFFSET);
> +	else if (dev->desc == &lradc->isrc1)
> +		return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC1_OFFSET);
> +
> +	/* This should never happen */
> +	return -ENODEV;
> +}
> +
> +static int mxs_lradc_regulator_enable(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
> +				  LRADC_CTRL2);
> +	else if (dev->desc == &lradc->isrc1)
> +		mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
> +				  LRADC_CTRL2);
> +	else
> +		/* This should never happen */
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int mxs_lradc_regulator_disable(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
> +				    LRADC_CTRL2);
> +	else if (dev->desc == &lradc->isrc1)
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
> +				    LRADC_CTRL2);
> +	else
> +		/* This should never happen */
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int mxs_lradc_regulator_set_current_limit(struct regulator_dev *dev,
> +						 int min_uA, int max_uA)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +	int offset, value;
> +
> +	if (dev->desc == &lradc->isrc0)
> +		offset = LRADC_CTRL2_TEMP_ISRC0_OFFSET;
> +	else if (dev->desc == &lradc->isrc1)
> +		offset = LRADC_CTRL2_TEMP_ISRC1_OFFSET;
> +	else
> +		/* This should never happen */
> +		return -ENODEV;
> +
> +	value = min_uA / 20;
> +	if (min_uA % 20)
> +		value++;
> +	if (value * 20 > max_uA)
> +		return -EINVAL;
> +	if (value & ~LRADC_CTRL2_TEMP_ISRC_MASK)
> +		/* This should never happen */
> +		return -EPERM;
> +
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_ISRC_MASK << offset,
> +			    LRADC_CTRL2);
> +	mxs_lradc_reg_set(lradc, value << offset, LRADC_CTRL2);
> +
> +	return 0;
> +}
> +
> +static struct regulator_ops mxs_lradc_regulator_current_ops = {
> +	.enable = mxs_lradc_regulator_enable,
> +	.is_enabled = mxs_lradc_regulator_is_enabled,
> +	.disable = mxs_lradc_regulator_disable,
> +	.get_current_limit = mxs_lradc_regulator_get_current_limit,
> +	.set_current_limit = mxs_lradc_regulator_set_current_limit,
> +};
> +
>  /*
>   * Driver initialization
>   */
> @@ -1519,6 +1634,10 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
>  
>  	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>  		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
> +
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0 |
> +					LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
> +			    LRADC_CTRL2);
>  }
>  
>  static const struct of_device_id mxs_lradc_dt_ids[] = {
> @@ -1592,6 +1711,32 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>  	return 0;
>  }
>  
> +static void mxs_lradc_reg_helper(struct device_node *np, const char *name,
> +				 struct regulator_config *conf,
> +				 struct regulator_desc *desc)
> +{
> +	struct regulator_dev *ret;
> +
> +	conf->of_node = of_get_child_by_name(np, name);
> +	if (!conf->of_node)
> +		return;
> +
> +	desc->name = name;
> +	desc->owner = THIS_MODULE;
> +	desc->type = REGULATOR_CURRENT;
> +	desc->ops = &mxs_lradc_regulator_current_ops;
> +
> +	conf->init_data = of_get_regulator_init_data(conf->dev, conf->of_node,
> +						     desc);
> +	ret = devm_regulator_register(conf->dev, desc, conf);
> +	if (IS_ERR(ret))
> +		/* Just pretend the regulator isn't there */
> +		dev_err(conf->dev, "Failed to register regulator %s: %ld\n",
> +			desc->name, PTR_ERR(ret));
> +
> +	of_node_put(conf->of_node);
> +}
> +
>  static int mxs_lradc_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *of_id =
> @@ -1603,6 +1748,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>  	struct mxs_lradc *lradc;
>  	struct iio_dev *iio;
>  	struct resource *iores;
> +	struct regulator_config regconf;
>  	int ret = 0, touch_ret;
>  	int i, s;
>  	u64 scale_uv;
> @@ -1727,6 +1873,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>  		goto err_ts;
>  	}
>  
> +	/* Setup regulator devices for current source. */
> +	regconf.dev = dev;
> +	regconf.driver_data = lradc;
> +	mxs_lradc_reg_helper(node, "ISRC0", &regconf, &lradc->isrc0);
> +	mxs_lradc_reg_helper(node, "ISRC1", &regconf, &lradc->isrc1);
> +
>  	return 0;
>  
>  err_ts:
> 


-- 
Best regards,
Marek Vasut

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Ksenija Stanojević]

HI

On Fri, Apr 22, 2016 at 5:50 PM, Marek Vasut <marex@denx.de> wrote:
> On 04/22/2016 03:52 PM, Harald Geyer wrote:
>> The hardware has two current sources ISRC0 and ISRC1 to allow measuring
>> resistors without additional circuitry. This commit makes them available
>> as regulators.
>>
>> Tested on an imx233-olinuxino board.
>>
>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>> ---
>> The current regulator API doesn't fit this type of device very well: Typically
>> consumers will want to set a defined current, ie. min_uA == max_uA, but they
>> can't without help from configuration data, because the valid values aren't
>> reported by the API for current regulators. I have been thinking about
>> extending the API, but currently AFAIK no such consumers exist and most
>> users, like myself, will force the regulator to a defined value in
>> devicetree anyway.
>
> I am tempted to block this patch and ask you to properly split the
> mxs-lradc driver into MFD with touchscreen and IIO part and only then
> add the regulator bits. The lradc is becoming a katamari of ad-hoc
> misplaced functionality.

I almost finished, I already split the driver into MFD, sorry for the delay.
Can this wait a little bit?

Thanks,
Ksenija

>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>  drivers/iio/adc/Kconfig                            |   1 +
>>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>>  3 files changed, 182 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>> index 555fb11..983952c 100644
>> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>> @@ -19,6 +19,15 @@ Optional properties:
>>  - fsl,settling: delay between plate switch to next sample. Allowed value is
>>                  1 ... 2047. It counts at 2 kHz and its default is
>>                  10 (= 5 ms)
>> +- ISRC0: A node describing the regulator of internal current source 0
>> +- ISRC1: A node describing the regulator of internal current source 1
>> +
>> +Required properties for the ISRCx sub-nodes:
>> +- regulator-max-microamp: See standard regulator binding documentation.
>> +                          Valid values are from 0 to 300 in steps of 20.
>> +
>> +Optional properties for the ISRCx sub-nodes:
>> +Any standard regulator properties that apply to current regulators.
>>
>>  Example for i.MX23 SoC:
>>
>> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>>               fsl,ave-ctrl = <4>;
>>               fsl,ave-delay = <2>;
>>               fsl,settling = <10>;
>> +
>> +             isrc_lradc0: ISRC0 {
>> +                     regulator-max-microamp = <300>;
>> +             };
>> +
>> +             isrc_lradc1: ISRC1 {
>> +                     regulator-max-microamp = <120>;
>> +                     regulator-min-microamp = <120>;
>> +                     regulator-always-on;
>> +             };
>>       };
>>
>>  Example for i.MX28 SoC:
>> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>>               fsl,ave-ctrl = <4>;
>>               fsl,ave-delay = <2>;
>>               fsl,settling = <10>;
>> +
>> +             isrc_lradc0: ISRC0 {
>> +                     regulator-max-microamp = <300>;
>> +             };
>> +
>> +             isrc_lradc6: ISRC1 {
>> +                     regulator-max-microamp = <120>;
>> +                     regulator-min-microamp = <120>;
>> +                     regulator-always-on;
>> +             };
>>       };
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 5937030..1968d1c 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -319,6 +319,7 @@ config MXS_LRADC
>>          tristate "Freescale i.MX23/i.MX28 LRADC"
>>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>>          depends on INPUT
>> +        depends on REGULATOR
>>          select STMP_DEVICE
>>          select IIO_BUFFER
>>          select IIO_TRIGGERED_BUFFER
>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>> index 33051b8..f22f339 100644
>> --- a/drivers/iio/adc/mxs-lradc.c
>> +++ b/drivers/iio/adc/mxs-lradc.c
>> @@ -40,6 +40,10 @@
>>  #include <linux/iio/triggered_buffer.h>
>>  #include <linux/iio/sysfs.h>
>>
>> +#include <linux/regulator/driver.h>
>> +#include <linux/regulator/machine.h>
>> +#include <linux/regulator/of_regulator.h>
>> +
>>  #define DRIVER_NAME          "mxs-lradc"
>>
>>  #define LRADC_MAX_DELAY_CHANS        4
>> @@ -261,6 +265,9 @@ struct mxs_lradc {
>>       unsigned                over_sample_delay;
>>       /* time in clocks to wait after the plates where switched */
>>       unsigned                settling_delay;
>> +
>> +     struct regulator_desc isrc0;
>> +     struct regulator_desc isrc1;
>>  };
>>
>>  #define      LRADC_CTRL0                             0x00
>> @@ -305,6 +312,11 @@ struct mxs_lradc {
>>  #define      LRADC_CTRL2                             0x20
>>  #define      LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET        24
>>  #define      LRADC_CTRL2_TEMPSENSE_PWD               BIT(15)
>> +#define      LRADC_CTRL2_TEMP_SENSOR_IENABLE1        BIT(9)
>> +#define      LRADC_CTRL2_TEMP_SENSOR_IENABLE0        BIT(8)
>> +#define      LRADC_CTRL2_TEMP_ISRC1_OFFSET           4
>> +#define      LRADC_CTRL2_TEMP_ISRC0_OFFSET           0
>> +#define      LRADC_CTRL2_TEMP_ISRC_MASK              0x0f
>>
>>  #define      LRADC_STATUS                            0x40
>>  #define      LRADC_STATUS_TOUCH_DETECT_RAW           BIT(0)
>> @@ -1383,6 +1395,109 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
>>       .validate_scan_mask = &mxs_lradc_validate_scan_mask,
>>  };
>>
>> +static int mxs_lradc_regulator_is_enabled(struct regulator_dev *dev)
>> +{
>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>> +     int reg = readl(lradc->base + LRADC_CTRL2);
>> +
>> +     if (dev->desc == &lradc->isrc0)
>> +             return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE0;
>> +     else if (dev->desc == &lradc->isrc1)
>> +             return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE1;
>> +
>> +     /* This should never happen */
>> +     return -ENODEV;
>> +}
>> +
>> +#define LRADC_REGVALUE2uA(regval, offset) \
>> +     (20 * ((regval >> offset) & LRADC_CTRL2_TEMP_ISRC_MASK))
>> +
>> +static int mxs_lradc_regulator_get_current_limit(struct regulator_dev *dev)
>> +{
>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>> +     int reg = readl(lradc->base + LRADC_CTRL2);
>> +
>> +     if (dev->desc == &lradc->isrc0)
>> +             return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC0_OFFSET);
>> +     else if (dev->desc == &lradc->isrc1)
>> +             return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC1_OFFSET);
>> +
>> +     /* This should never happen */
>> +     return -ENODEV;
>> +}
>> +
>> +static int mxs_lradc_regulator_enable(struct regulator_dev *dev)
>> +{
>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>> +
>> +     if (dev->desc == &lradc->isrc0)
>> +             mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
>> +                               LRADC_CTRL2);
>> +     else if (dev->desc == &lradc->isrc1)
>> +             mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>> +                               LRADC_CTRL2);
>> +     else
>> +             /* This should never happen */
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int mxs_lradc_regulator_disable(struct regulator_dev *dev)
>> +{
>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>> +
>> +     if (dev->desc == &lradc->isrc0)
>> +             mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
>> +                                 LRADC_CTRL2);
>> +     else if (dev->desc == &lradc->isrc1)
>> +             mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>> +                                 LRADC_CTRL2);
>> +     else
>> +             /* This should never happen */
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int mxs_lradc_regulator_set_current_limit(struct regulator_dev *dev,
>> +                                              int min_uA, int max_uA)
>> +{
>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>> +     int offset, value;
>> +
>> +     if (dev->desc == &lradc->isrc0)
>> +             offset = LRADC_CTRL2_TEMP_ISRC0_OFFSET;
>> +     else if (dev->desc == &lradc->isrc1)
>> +             offset = LRADC_CTRL2_TEMP_ISRC1_OFFSET;
>> +     else
>> +             /* This should never happen */
>> +             return -ENODEV;
>> +
>> +     value = min_uA / 20;
>> +     if (min_uA % 20)
>> +             value++;
>> +     if (value * 20 > max_uA)
>> +             return -EINVAL;
>> +     if (value & ~LRADC_CTRL2_TEMP_ISRC_MASK)
>> +             /* This should never happen */
>> +             return -EPERM;
>> +
>> +     mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_ISRC_MASK << offset,
>> +                         LRADC_CTRL2);
>> +     mxs_lradc_reg_set(lradc, value << offset, LRADC_CTRL2);
>> +
>> +     return 0;
>> +}
>> +
>> +static struct regulator_ops mxs_lradc_regulator_current_ops = {
>> +     .enable = mxs_lradc_regulator_enable,
>> +     .is_enabled = mxs_lradc_regulator_is_enabled,
>> +     .disable = mxs_lradc_regulator_disable,
>> +     .get_current_limit = mxs_lradc_regulator_get_current_limit,
>> +     .set_current_limit = mxs_lradc_regulator_set_current_limit,
>> +};
>> +
>>  /*
>>   * Driver initialization
>>   */
>> @@ -1519,6 +1634,10 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
>>
>>       for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>>               mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
>> +
>> +     mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0 |
>> +                                     LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>> +                         LRADC_CTRL2);
>>  }
>>
>>  static const struct of_device_id mxs_lradc_dt_ids[] = {
>> @@ -1592,6 +1711,32 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>>       return 0;
>>  }
>>
>> +static void mxs_lradc_reg_helper(struct device_node *np, const char *name,
>> +                              struct regulator_config *conf,
>> +                              struct regulator_desc *desc)
>> +{
>> +     struct regulator_dev *ret;
>> +
>> +     conf->of_node = of_get_child_by_name(np, name);
>> +     if (!conf->of_node)
>> +             return;
>> +
>> +     desc->name = name;
>> +     desc->owner = THIS_MODULE;
>> +     desc->type = REGULATOR_CURRENT;
>> +     desc->ops = &mxs_lradc_regulator_current_ops;
>> +
>> +     conf->init_data = of_get_regulator_init_data(conf->dev, conf->of_node,
>> +                                                  desc);
>> +     ret = devm_regulator_register(conf->dev, desc, conf);
>> +     if (IS_ERR(ret))
>> +             /* Just pretend the regulator isn't there */
>> +             dev_err(conf->dev, "Failed to register regulator %s: %ld\n",
>> +                     desc->name, PTR_ERR(ret));
>> +
>> +     of_node_put(conf->of_node);
>> +}
>> +
>>  static int mxs_lradc_probe(struct platform_device *pdev)
>>  {
>>       const struct of_device_id *of_id =
>> @@ -1603,6 +1748,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>>       struct mxs_lradc *lradc;
>>       struct iio_dev *iio;
>>       struct resource *iores;
>> +     struct regulator_config regconf;
>>       int ret = 0, touch_ret;
>>       int i, s;
>>       u64 scale_uv;
>> @@ -1727,6 +1873,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>>               goto err_ts;
>>       }
>>
>> +     /* Setup regulator devices for current source. */
>> +     regconf.dev = dev;
>> +     regconf.driver_data = lradc;
>> +     mxs_lradc_reg_helper(node, "ISRC0", &regconf, &lradc->isrc0);
>> +     mxs_lradc_reg_helper(node, "ISRC1", &regconf, &lradc->isrc1);
>> +
>>       return 0;
>>
>>  err_ts:
>>
>
>
> --
> Best regards,
> Marek Vasut
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Harald Geyer]

[CCing Mark and Liam again]

On 22.04.2016 19:00, Ksenija Stanojevi? wrote:
> On Fri, Apr 22, 2016 at 5:50 PM, Marek Vasut <marex@denx.de> wrote:
>> On 04/22/2016 03:52 PM, Harald Geyer wrote:
>>> The hardware has two current sources ISRC0 and ISRC1 to allow 
>>> measuring
>>> resistors without additional circuitry. This commit makes them 
>>> available
>>> as regulators.
>>>
>>> Tested on an imx233-olinuxino board.
>>>
>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>>> ---
>>> The current regulator API doesn't fit this type of device very 
>>> well: Typically
>>> consumers will want to set a defined current, ie. min_uA == max_uA, 
>>> but they
>>> can't without help from configuration data, because the valid 
>>> values aren't
>>> reported by the API for current regulators. I have been thinking 
>>> about
>>> extending the API, but currently AFAIK no such consumers exist and 
>>> most
>>> users, like myself, will force the regulator to a defined value in
>>> devicetree anyway.
>>
>> I am tempted to block this patch and ask you to properly split the
>> mxs-lradc driver into MFD with touchscreen and IIO part and only 
>> then
>> add the regulator bits. The lradc is becoming a katamari of ad-hoc
>> misplaced functionality.

I know. However I'm afraid working this into a proper MFD is above my 
skills
and I don't feel responsible for the status quo either.

> I almost finished, I already split the driver into MFD, sorry for the 
> delay.

Wow, looks like I have a lot of luck today.

> Can this wait a little bit?

Sure. I now have a working kernel for my purposes, so no hurry on my 
part.
CC my when you submit your code or maybe even pick my patch into your 
series,
if you feel like it.

Thanks,
Harald

> Thanks,
> Ksenija
>
>>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>>  drivers/iio/adc/Kconfig                            |   1 +
>>>  drivers/iio/adc/mxs-lradc.c                        | 152 
>>> +++++++++++++++++++++
>>>  3 files changed, 182 insertions(+)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt 
>>> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> index 555fb11..983952c 100644
>>> --- 
>>> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> +++ 
>>> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> @@ -19,6 +19,15 @@ Optional properties:
>>>  - fsl,settling: delay between plate switch to next sample. Allowed 
>>> value is
>>>                  1 ... 2047. It counts at 2 kHz and its default is
>>>                  10 (= 5 ms)
>>> +- ISRC0: A node describing the regulator of internal current 
>>> source 0
>>> +- ISRC1: A node describing the regulator of internal current 
>>> source 1
>>> +
>>> +Required properties for the ISRCx sub-nodes:
>>> +- regulator-max-microamp: See standard regulator binding 
>>> documentation.
>>> +                          Valid values are from 0 to 300 in steps 
>>> of 20.
>>> +
>>> +Optional properties for the ISRCx sub-nodes:
>>> +Any standard regulator properties that apply to current 
>>> regulators.
>>>
>>>  Example for i.MX23 SoC:
>>>
>>> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>>>               fsl,ave-ctrl = <4>;
>>>               fsl,ave-delay = <2>;
>>>               fsl,settling = <10>;
>>> +
>>> +             isrc_lradc0: ISRC0 {
>>> +                     regulator-max-microamp = <300>;
>>> +             };
>>> +
>>> +             isrc_lradc1: ISRC1 {
>>> +                     regulator-max-microamp = <120>;
>>> +                     regulator-min-microamp = <120>;
>>> +                     regulator-always-on;
>>> +             };
>>>       };
>>>
>>>  Example for i.MX28 SoC:
>>> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>>>               fsl,ave-ctrl = <4>;
>>>               fsl,ave-delay = <2>;
>>>               fsl,settling = <10>;
>>> +
>>> +             isrc_lradc0: ISRC0 {
>>> +                     regulator-max-microamp = <300>;
>>> +             };
>>> +
>>> +             isrc_lradc6: ISRC1 {
>>> +                     regulator-max-microamp = <120>;
>>> +                     regulator-min-microamp = <120>;
>>> +                     regulator-always-on;
>>> +             };
>>>       };
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 5937030..1968d1c 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -319,6 +319,7 @@ config MXS_LRADC
>>>          tristate "Freescale i.MX23/i.MX28 LRADC"
>>>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>>>          depends on INPUT
>>> +        depends on REGULATOR
>>>          select STMP_DEVICE
>>>          select IIO_BUFFER
>>>          select IIO_TRIGGERED_BUFFER
>>> diff --git a/drivers/iio/adc/mxs-lradc.c 
>>> b/drivers/iio/adc/mxs-lradc.c
>>> index 33051b8..f22f339 100644
>>> --- a/drivers/iio/adc/mxs-lradc.c
>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>> @@ -40,6 +40,10 @@
>>>  #include <linux/iio/triggered_buffer.h>
>>>  #include <linux/iio/sysfs.h>
>>>
>>> +#include <linux/regulator/driver.h>
>>> +#include <linux/regulator/machine.h>
>>> +#include <linux/regulator/of_regulator.h>
>>> +
>>>  #define DRIVER_NAME          "mxs-lradc"
>>>
>>>  #define LRADC_MAX_DELAY_CHANS        4
>>> @@ -261,6 +265,9 @@ struct mxs_lradc {
>>>       unsigned                over_sample_delay;
>>>       /* time in clocks to wait after the plates where switched */
>>>       unsigned                settling_delay;
>>> +
>>> +     struct regulator_desc isrc0;
>>> +     struct regulator_desc isrc1;
>>>  };
>>>
>>>  #define      LRADC_CTRL0                             0x00
>>> @@ -305,6 +312,11 @@ struct mxs_lradc {
>>>  #define      LRADC_CTRL2                             0x20
>>>  #define      LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET        24
>>>  #define      LRADC_CTRL2_TEMPSENSE_PWD               BIT(15)
>>> +#define      LRADC_CTRL2_TEMP_SENSOR_IENABLE1        BIT(9)
>>> +#define      LRADC_CTRL2_TEMP_SENSOR_IENABLE0        BIT(8)
>>> +#define      LRADC_CTRL2_TEMP_ISRC1_OFFSET           4
>>> +#define      LRADC_CTRL2_TEMP_ISRC0_OFFSET           0
>>> +#define      LRADC_CTRL2_TEMP_ISRC_MASK              0x0f
>>>
>>>  #define      LRADC_STATUS                            0x40
>>>  #define      LRADC_STATUS_TOUCH_DETECT_RAW           BIT(0)
>>> @@ -1383,6 +1395,109 @@ static const struct iio_buffer_setup_ops 
>>> mxs_lradc_buffer_ops = {
>>>       .validate_scan_mask = &mxs_lradc_validate_scan_mask,
>>>  };
>>>
>>> +static int mxs_lradc_regulator_is_enabled(struct regulator_dev 
>>> *dev)
>>> +{
>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>> +     int reg = readl(lradc->base + LRADC_CTRL2);
>>> +
>>> +     if (dev->desc == &lradc->isrc0)
>>> +             return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE0;
>>> +     else if (dev->desc == &lradc->isrc1)
>>> +             return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE1;
>>> +
>>> +     /* This should never happen */
>>> +     return -ENODEV;
>>> +}
>>> +
>>> +#define LRADC_REGVALUE2uA(regval, offset) \
>>> +     (20 * ((regval >> offset) & LRADC_CTRL2_TEMP_ISRC_MASK))
>>> +
>>> +static int mxs_lradc_regulator_get_current_limit(struct 
>>> regulator_dev *dev)
>>> +{
>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>> +     int reg = readl(lradc->base + LRADC_CTRL2);
>>> +
>>> +     if (dev->desc == &lradc->isrc0)
>>> +             return LRADC_REGVALUE2uA(reg, 
>>> LRADC_CTRL2_TEMP_ISRC0_OFFSET);
>>> +     else if (dev->desc == &lradc->isrc1)
>>> +             return LRADC_REGVALUE2uA(reg, 
>>> LRADC_CTRL2_TEMP_ISRC1_OFFSET);
>>> +
>>> +     /* This should never happen */
>>> +     return -ENODEV;
>>> +}
>>> +
>>> +static int mxs_lradc_regulator_enable(struct regulator_dev *dev)
>>> +{
>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>> +
>>> +     if (dev->desc == &lradc->isrc0)
>>> +             mxs_lradc_reg_set(lradc, 
>>> LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
>>> +                               LRADC_CTRL2);
>>> +     else if (dev->desc == &lradc->isrc1)
>>> +             mxs_lradc_reg_set(lradc, 
>>> LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>>> +                               LRADC_CTRL2);
>>> +     else
>>> +             /* This should never happen */
>>> +             return -ENODEV;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int mxs_lradc_regulator_disable(struct regulator_dev *dev)
>>> +{
>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>> +
>>> +     if (dev->desc == &lradc->isrc0)
>>> +             mxs_lradc_reg_clear(lradc, 
>>> LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
>>> +                                 LRADC_CTRL2);
>>> +     else if (dev->desc == &lradc->isrc1)
>>> +             mxs_lradc_reg_clear(lradc, 
>>> LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>>> +                                 LRADC_CTRL2);
>>> +     else
>>> +             /* This should never happen */
>>> +             return -ENODEV;
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int mxs_lradc_regulator_set_current_limit(struct 
>>> regulator_dev *dev,
>>> +                                              int min_uA, int 
>>> max_uA)
>>> +{
>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>> +     int offset, value;
>>> +
>>> +     if (dev->desc == &lradc->isrc0)
>>> +             offset = LRADC_CTRL2_TEMP_ISRC0_OFFSET;
>>> +     else if (dev->desc == &lradc->isrc1)
>>> +             offset = LRADC_CTRL2_TEMP_ISRC1_OFFSET;
>>> +     else
>>> +             /* This should never happen */
>>> +             return -ENODEV;
>>> +
>>> +     value = min_uA / 20;
>>> +     if (min_uA % 20)
>>> +             value++;
>>> +     if (value * 20 > max_uA)
>>> +             return -EINVAL;
>>> +     if (value & ~LRADC_CTRL2_TEMP_ISRC_MASK)
>>> +             /* This should never happen */
>>> +             return -EPERM;
>>> +
>>> +     mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_ISRC_MASK << 
>>> offset,
>>> +                         LRADC_CTRL2);
>>> +     mxs_lradc_reg_set(lradc, value << offset, LRADC_CTRL2);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static struct regulator_ops mxs_lradc_regulator_current_ops = {
>>> +     .enable = mxs_lradc_regulator_enable,
>>> +     .is_enabled = mxs_lradc_regulator_is_enabled,
>>> +     .disable = mxs_lradc_regulator_disable,
>>> +     .get_current_limit = mxs_lradc_regulator_get_current_limit,
>>> +     .set_current_limit = mxs_lradc_regulator_set_current_limit,
>>> +};
>>> +
>>>  /*
>>>   * Driver initialization
>>>   */
>>> @@ -1519,6 +1634,10 @@ static void mxs_lradc_hw_stop(struct 
>>> mxs_lradc *lradc)
>>>
>>>       for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>>>               mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
>>> +
>>> +     mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0 |
>>> +                                     
>>> LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>>> +                         LRADC_CTRL2);
>>>  }
>>>
>>>  static const struct of_device_id mxs_lradc_dt_ids[] = {
>>> @@ -1592,6 +1711,32 @@ static int 
>>> mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>>>       return 0;
>>>  }
>>>
>>> +static void mxs_lradc_reg_helper(struct device_node *np, const 
>>> char *name,
>>> +                              struct regulator_config *conf,
>>> +                              struct regulator_desc *desc)
>>> +{
>>> +     struct regulator_dev *ret;
>>> +
>>> +     conf->of_node = of_get_child_by_name(np, name);
>>> +     if (!conf->of_node)
>>> +             return;
>>> +
>>> +     desc->name = name;
>>> +     desc->owner = THIS_MODULE;
>>> +     desc->type = REGULATOR_CURRENT;
>>> +     desc->ops = &mxs_lradc_regulator_current_ops;
>>> +
>>> +     conf->init_data = of_get_regulator_init_data(conf->dev, 
>>> conf->of_node,
>>> +                                                  desc);
>>> +     ret = devm_regulator_register(conf->dev, desc, conf);
>>> +     if (IS_ERR(ret))
>>> +             /* Just pretend the regulator isn't there */
>>> +             dev_err(conf->dev, "Failed to register regulator %s: 
>>> %ld\n",
>>> +                     desc->name, PTR_ERR(ret));
>>> +
>>> +     of_node_put(conf->of_node);
>>> +}
>>> +
>>>  static int mxs_lradc_probe(struct platform_device *pdev)
>>>  {
>>>       const struct of_device_id *of_id =
>>> @@ -1603,6 +1748,7 @@ static int mxs_lradc_probe(struct 
>>> platform_device *pdev)
>>>       struct mxs_lradc *lradc;
>>>       struct iio_dev *iio;
>>>       struct resource *iores;
>>> +     struct regulator_config regconf;
>>>       int ret = 0, touch_ret;
>>>       int i, s;
>>>       u64 scale_uv;
>>> @@ -1727,6 +1873,12 @@ static int mxs_lradc_probe(struct 
>>> platform_device *pdev)
>>>               goto err_ts;
>>>       }
>>>
>>> +     /* Setup regulator devices for current source. */
>>> +     regconf.dev = dev;
>>> +     regconf.driver_data = lradc;
>>> +     mxs_lradc_reg_helper(node, "ISRC0", &regconf, &lradc->isrc0);
>>> +     mxs_lradc_reg_helper(node, "ISRC1", &regconf, &lradc->isrc1);
>>> +
>>>       return 0;
>>>
>>>  err_ts:
>>>
>>
>>
>> --
>> Best regards,
>> Marek Vasut
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" 
>> in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or bitcoin 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Jonathan Cameron]

On 22/04/16 20:23, Harald Geyer wrote:
> [CCing Mark and Liam again]
> 
> On 22.04.2016 19:00, Ksenija Stanojevi? wrote:
>> On Fri, Apr 22, 2016 at 5:50 PM, Marek Vasut <marex@denx.de> wrote:
>>> On 04/22/2016 03:52 PM, Harald Geyer wrote:
>>>> The hardware has two current sources ISRC0 and ISRC1 to allow measuring
>>>> resistors without additional circuitry. This commit makes them available
>>>> as regulators.
>>>>
>>>> Tested on an imx233-olinuxino board.
>>>>
>>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>>>> ---
>>>> The current regulator API doesn't fit this type of device very well: Typically
>>>> consumers will want to set a defined current, ie. min_uA == max_uA, but they
>>>> can't without help from configuration data, because the valid values aren't
>>>> reported by the API for current regulators. I have been thinking about
>>>> extending the API, but currently AFAIK no such consumers exist and most
>>>> users, like myself, will force the regulator to a defined value in
>>>> devicetree anyway.
>>>
>>> I am tempted to block this patch and ask you to properly split the
>>> mxs-lradc driver into MFD with touchscreen and IIO part and only then
>>> add the regulator bits. The lradc is becoming a katamari of ad-hoc
>>> misplaced functionality.
> 
> I know. However I'm afraid working this into a proper MFD is above my skills
> and I don't feel responsible for the status quo either.
> 
>> I almost finished, I already split the driver into MFD, sorry for the delay.
>
> Wow, looks like I have a lot of luck today.
Indeed!  Glad to here you have this underway Ksenija.  Let me know if you want
any other patches held until this is done.

For now I'm assuming that Stefan's series won't cause too much trouble, so I'll
probably apply those, but will hold anything more major until your patches show
up.

Thanks,

Jonathan
> 
>> Can this wait a  little bit?
> 
> Sure. I now have a working kernel for my purposes, so no hurry on my part.
> CC my when you submit your code or maybe even pick my patch into your series,
> if you feel like it.
> 
> Thanks,
> Harald
> 
>> Thanks,
>> Ksenija
>>
>>>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>>>  drivers/iio/adc/Kconfig                            |   1 +
>>>>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>>>>  3 files changed, 182 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> index 555fb11..983952c 100644
>>>> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> @@ -19,6 +19,15 @@ Optional properties:
>>>>  - fsl,settling: delay between plate switch to next sample. Allowed value is
>>>>                  1 ... 2047. It counts at 2 kHz and its default is
>>>>                  10 (= 5 ms)
>>>> +- ISRC0: A node describing the regulator of internal current source 0
>>>> +- ISRC1: A node describing the regulator of internal current source 1
>>>> +
>>>> +Required properties for the ISRCx sub-nodes:
>>>> +- regulator-max-microamp: See standard regulator binding documentation.
>>>> +                          Valid values are from 0 to 300 in steps of 20.
>>>> +
>>>> +Optional properties for the ISRCx sub-nodes:
>>>> +Any standard regulator properties that apply to current regulators.
>>>>
>>>>  Example for i.MX23 SoC:
>>>>
>>>> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>>>>               fsl,ave-ctrl = <4>;
>>>>               fsl,ave-delay = <2>;
>>>>               fsl,settling = <10>;
>>>> +
>>>> +             isrc_lradc0: ISRC0 {
>>>> +                     regulator-max-microamp = <300>;
>>>> +             };
>>>> +
>>>> +             isrc_lradc1: ISRC1 {
>>>> +                     regulator-max-microamp = <120>;
>>>> +                     regulator-min-microamp = <120>;
>>>> +                     regulator-always-on;
>>>> +             };
>>>>       };
>>>>
>>>>  Example for i.MX28 SoC:
>>>> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>>>>               fsl,ave-ctrl = <4>;
>>>>               fsl,ave-delay = <2>;
>>>>               fsl,settling = <10>;
>>>> +
>>>> +             isrc_lradc0: ISRC0 {
>>>> +                     regulator-max-microamp = <300>;
>>>> +             };
>>>> +
>>>> +             isrc_lradc6: ISRC1 {
>>>> +                     regulator-max-microamp = <120>;
>>>> +                     regulator-min-microamp = <120>;
>>>> +                     regulator-always-on;
>>>> +             };
>>>>       };
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index 5937030..1968d1c 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -319,6 +319,7 @@ config MXS_LRADC
>>>>          tristate "Freescale i.MX23/i.MX28 LRADC"
>>>>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>>>>          depends on INPUT
>>>> +        depends on REGULATOR
>>>>          select STMP_DEVICE
>>>>          select IIO_BUFFER
>>>>          select IIO_TRIGGERED_BUFFER
>>>> diff --git a/drivers/iio/adc/mxs-lradc.c b/drivers/iio/adc/mxs-lradc.c
>>>> index 33051b8..f22f339 100644
>>>> --- a/drivers/iio/adc/mxs-lradc.c
>>>> +++ b/drivers/iio/adc/mxs-lradc.c
>>>> @@ -40,6 +40,10 @@
>>>>  #include <linux/iio/triggered_buffer.h>
>>>>  #include <linux/iio/sysfs.h>
>>>>
>>>> +#include <linux/regulator/driver.h>
>>>> +#include <linux/regulator/machine.h>
>>>> +#include <linux/regulator/of_regulator.h>
>>>> +
>>>>  #define DRIVER_NAME          "mxs-lradc"
>>>>
>>>>  #define LRADC_MAX_DELAY_CHANS        4
>>>> @@ -261,6 +265,9 @@ struct mxs_lradc {
>>>>       unsigned                over_sample_delay;
>>>>       /* time in clocks to wait after the plates where switched */
>>>>       unsigned                settling_delay;
>>>> +
>>>> +     struct regulator_desc isrc0;
>>>> +     struct regulator_desc isrc1;
>>>>  };
>>>>
>>>>  #define      LRADC_CTRL0                             0x00
>>>> @@ -305,6 +312,11 @@ struct mxs_lradc {
>>>>  #define      LRADC_CTRL2                             0x20
>>>>  #define      LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET        24
>>>>  #define      LRADC_CTRL2_TEMPSENSE_PWD               BIT(15)
>>>> +#define      LRADC_CTRL2_TEMP_SENSOR_IENABLE1        BIT(9)
>>>> +#define      LRADC_CTRL2_TEMP_SENSOR_IENABLE0        BIT(8)
>>>> +#define      LRADC_CTRL2_TEMP_ISRC1_OFFSET           4
>>>> +#define      LRADC_CTRL2_TEMP_ISRC0_OFFSET           0
>>>> +#define      LRADC_CTRL2_TEMP_ISRC_MASK              0x0f
>>>>
>>>>  #define      LRADC_STATUS                            0x40
>>>>  #define      LRADC_STATUS_TOUCH_DETECT_RAW           BIT(0)
>>>> @@ -1383,6 +1395,109 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
>>>>       .validate_scan_mask = &mxs_lradc_validate_scan_mask,
>>>>  };
>>>>
>>>> +static int mxs_lradc_regulator_is_enabled(struct regulator_dev *dev)
>>>> +{
>>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>>> +     int reg = readl(lradc->base + LRADC_CTRL2);
>>>> +
>>>> +     if (dev->desc == &lradc->isrc0)
>>>> +             return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE0;
>>>> +     else if (dev->desc == &lradc->isrc1)
>>>> +             return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE1;
>>>> +
>>>> +     /* This should never happen */
>>>> +     return -ENODEV;
>>>> +}
>>>> +
>>>> +#define LRADC_REGVALUE2uA(regval, offset) \
>>>> +     (20 * ((regval >> offset) & LRADC_CTRL2_TEMP_ISRC_MASK))
>>>> +
>>>> +static int mxs_lradc_regulator_get_current_limit(struct regulator_dev *dev)
>>>> +{
>>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>>> +     int reg = readl(lradc->base + LRADC_CTRL2);
>>>> +
>>>> +     if (dev->desc == &lradc->isrc0)
>>>> +             return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC0_OFFSET);
>>>> +     else if (dev->desc == &lradc->isrc1)
>>>> +             return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC1_OFFSET);
>>>> +
>>>> +     /* This should never happen */
>>>> +     return -ENODEV;
>>>> +}
>>>> +
>>>> +static int mxs_lradc_regulator_enable(struct regulator_dev *dev)
>>>> +{
>>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>>> +
>>>> +     if (dev->desc == &lradc->isrc0)
>>>> +             mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
>>>> +                               LRADC_CTRL2);
>>>> +     else if (dev->desc == &lradc->isrc1)
>>>> +             mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>>>> +                               LRADC_CTRL2);
>>>> +     else
>>>> +             /* This should never happen */
>>>> +             return -ENODEV;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int mxs_lradc_regulator_disable(struct regulator_dev *dev)
>>>> +{
>>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>>> +
>>>> +     if (dev->desc == &lradc->isrc0)
>>>> +             mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
>>>> +                                 LRADC_CTRL2);
>>>> +     else if (dev->desc == &lradc->isrc1)
>>>> +             mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>>>> +                                 LRADC_CTRL2);
>>>> +     else
>>>> +             /* This should never happen */
>>>> +             return -ENODEV;
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static int mxs_lradc_regulator_set_current_limit(struct regulator_dev *dev,
>>>> +                                              int min_uA, int max_uA)
>>>> +{
>>>> +     struct mxs_lradc *lradc = rdev_get_drvdata(dev);
>>>> +     int offset, value;
>>>> +
>>>> +     if (dev->desc == &lradc->isrc0)
>>>> +             offset = LRADC_CTRL2_TEMP_ISRC0_OFFSET;
>>>> +     else if (dev->desc == &lradc->isrc1)
>>>> +             offset = LRADC_CTRL2_TEMP_ISRC1_OFFSET;
>>>> +     else
>>>> +             /* This should never happen */
>>>> +             return -ENODEV;
>>>> +
>>>> +     value = min_uA / 20;
>>>> +     if (min_uA % 20)
>>>> +             value++;
>>>> +     if (value * 20 > max_uA)
>>>> +             return -EINVAL;
>>>> +     if (value & ~LRADC_CTRL2_TEMP_ISRC_MASK)
>>>> +             /* This should never happen */
>>>> +             return -EPERM;
>>>> +
>>>> +     mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_ISRC_MASK << offset,
>>>> +                         LRADC_CTRL2);
>>>> +     mxs_lradc_reg_set(lradc, value << offset, LRADC_CTRL2);
>>>> +
>>>> +     return 0;
>>>> +}
>>>> +
>>>> +static struct regulator_ops mxs_lradc_regulator_current_ops = {
>>>> +     .enable = mxs_lradc_regulator_enable,
>>>> +     .is_enabled = mxs_lradc_regulator_is_enabled,
>>>> +     .disable = mxs_lradc_regulator_disable,
>>>> +     .get_current_limit = mxs_lradc_regulator_get_current_limit,
>>>> +     .set_current_limit = mxs_lradc_regulator_set_current_limit,
>>>> +};
>>>> +
>>>>  /*
>>>>   * Driver initialization
>>>>   */
>>>> @@ -1519,6 +1634,10 @@ static void mxs_lradc_hw_stop(struct mxs_lradc *lradc)
>>>>
>>>>       for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>>>>               mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
>>>> +
>>>> +     mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0 |
>>>> +                                     LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
>>>> +                         LRADC_CTRL2);
>>>>  }
>>>>
>>>>  static const struct of_device_id mxs_lradc_dt_ids[] = {
>>>> @@ -1592,6 +1711,32 @@ static int mxs_lradc_probe_touchscreen(struct mxs_lradc *lradc,
>>>>       return 0;
>>>>  }
>>>>
>>>> +static void mxs_lradc_reg_helper(struct device_node *np, const char *name,
>>>> +                              struct regulator_config *conf,
>>>> +                              struct regulator_desc *desc)
>>>> +{
>>>> +     struct regulator_dev *ret;
>>>> +
>>>> +     conf->of_node = of_get_child_by_name(np, name);
>>>> +     if (!conf->of_node)
>>>> +             return;
>>>> +
>>>> +     desc->name = name;
>>>> +     desc->owner = THIS_MODULE;
>>>> +     desc->type = REGULATOR_CURRENT;
>>>> +     desc->ops = &mxs_lradc_regulator_current_ops;
>>>> +
>>>> +     conf->init_data = of_get_regulator_init_data(conf->dev, conf->of_node,
>>>> +                                                  desc);
>>>> +     ret = devm_regulator_register(conf->dev, desc, conf);
>>>> +     if (IS_ERR(ret))
>>>> +             /* Just pretend the regulator isn't there */
>>>> +             dev_err(conf->dev, "Failed to register regulator %s: %ld\n",
>>>> +                     desc->name, PTR_ERR(ret));
>>>> +
>>>> +     of_node_put(conf->of_node);
>>>> +}
>>>> +
>>>>  static int mxs_lradc_probe(struct platform_device *pdev)
>>>>  {
>>>>       const struct of_device_id *of_id =
>>>> @@ -1603,6 +1748,7 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>>>>       struct mxs_lradc *lradc;
>>>>       struct iio_dev *iio;
>>>>       struct resource *iores;
>>>> +     struct regulator_config regconf;
>>>>       int ret = 0, touch_ret;
>>>>       int i, s;
>>>>       u64 scale_uv;
>>>> @@ -1727,6 +1873,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
>>>>               goto err_ts;
>>>>       }
>>>>
>>>> +     /* Setup regulator devices for current source. */
>>>> +     regconf.dev = dev;
>>>> +     regconf.driver_data = lradc;
>>>> +     mxs_lradc_reg_helper(node, "ISRC0", &regconf, &lradc->isrc0);
>>>> +     mxs_lradc_reg_helper(node, "ISRC1", &regconf, &lradc->isrc1);
>>>> +
>>>>       return 0;
>>>>
>>>>  err_ts:
>>>>
>>>
>>>
>>> -- 
>>> Best regards,
>>> Marek Vasut
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo at vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Harald Geyer]

CCing Mark and Liam.

I guess I shouldn't rely blindly on get_maintainer.pl ...

On 22.04.2016 15:52, Harald Geyer wrote:
> The hardware has two current sources ISRC0 and ISRC1 to allow 
> measuring
> resistors without additional circuitry. This commit makes them 
> available
> as regulators.
>
> Tested on an imx233-olinuxino board.
>
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> The current regulator API doesn't fit this type of device very well:
> Typically
> consumers will want to set a defined current, ie. min_uA == max_uA, 
> but they
> can't without help from configuration data, because the valid values 
> aren't
> reported by the API for current regulators. I have been thinking 
> about
> extending the API, but currently AFAIK no such consumers exist and 
> most
> users, like myself, will force the regulator to a defined value in
> devicetree anyway.
>
>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>  drivers/iio/adc/Kconfig                            |   1 +
>  drivers/iio/adc/mxs-lradc.c                        | 152
> +++++++++++++++++++++
>  3 files changed, 182 insertions(+)
>
> diff --git
> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> index 555fb11..983952c 100644
> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> @@ -19,6 +19,15 @@ Optional properties:
>  - fsl,settling: delay between plate switch to next sample. Allowed 
> value is
>                  1 ... 2047. It counts at 2 kHz and its default is
>                  10 (= 5 ms)
> +- ISRC0: A node describing the regulator of internal current source 
> 0
> +- ISRC1: A node describing the regulator of internal current source 
> 1
> +
> +Required properties for the ISRCx sub-nodes:
> +- regulator-max-microamp: See standard regulator binding 
> documentation.
> +                          Valid values are from 0 to 300 in steps of 
> 20.
> +
> +Optional properties for the ISRCx sub-nodes:
> +Any standard regulator properties that apply to current regulators.
>
>  Example for i.MX23 SoC:
>
> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>  		fsl,ave-ctrl = <4>;
>  		fsl,ave-delay = <2>;
>  		fsl,settling = <10>;
> +
> +		isrc_lradc0: ISRC0 {
> +			regulator-max-microamp = <300>;
> +		};
> +
> +		isrc_lradc1: ISRC1 {
> +			regulator-max-microamp = <120>;
> +			regulator-min-microamp = <120>;
> +			regulator-always-on;
> +		};
>  	};
>
>  Example for i.MX28 SoC:
> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>  		fsl,ave-ctrl = <4>;
>  		fsl,ave-delay = <2>;
>  		fsl,settling = <10>;
> +
> +		isrc_lradc0: ISRC0 {
> +			regulator-max-microamp = <300>;
> +		};
> +
> +		isrc_lradc6: ISRC1 {
> +			regulator-max-microamp = <120>;
> +			regulator-min-microamp = <120>;
> +			regulator-always-on;
> +		};
>  	};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5937030..1968d1c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -319,6 +319,7 @@ config MXS_LRADC
>          tristate "Freescale i.MX23/i.MX28 LRADC"
>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>          depends on INPUT
> +        depends on REGULATOR
>          select STMP_DEVICE
>          select IIO_BUFFER
>          select IIO_TRIGGERED_BUFFER
> diff --git a/drivers/iio/adc/mxs-lradc.c 
> b/drivers/iio/adc/mxs-lradc.c
> index 33051b8..f22f339 100644
> --- a/drivers/iio/adc/mxs-lradc.c
> +++ b/drivers/iio/adc/mxs-lradc.c
> @@ -40,6 +40,10 @@
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/iio/sysfs.h>
>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/machine.h>
> +#include <linux/regulator/of_regulator.h>
> +
>  #define DRIVER_NAME		"mxs-lradc"
>
>  #define LRADC_MAX_DELAY_CHANS	4
> @@ -261,6 +265,9 @@ struct mxs_lradc {
>  	unsigned		over_sample_delay;
>  	/* time in clocks to wait after the plates where switched */
>  	unsigned		settling_delay;
> +
> +	struct regulator_desc isrc0;
> +	struct regulator_desc isrc1;
>  };
>
>  #define	LRADC_CTRL0				0x00
> @@ -305,6 +312,11 @@ struct mxs_lradc {
>  #define	LRADC_CTRL2				0x20
>  #define	LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET	24
>  #define	LRADC_CTRL2_TEMPSENSE_PWD		BIT(15)
> +#define	LRADC_CTRL2_TEMP_SENSOR_IENABLE1	BIT(9)
> +#define	LRADC_CTRL2_TEMP_SENSOR_IENABLE0	BIT(8)
> +#define	LRADC_CTRL2_TEMP_ISRC1_OFFSET		4
> +#define	LRADC_CTRL2_TEMP_ISRC0_OFFSET		0
> +#define	LRADC_CTRL2_TEMP_ISRC_MASK		0x0f
>
>  #define	LRADC_STATUS				0x40
>  #define	LRADC_STATUS_TOUCH_DETECT_RAW		BIT(0)
> @@ -1383,6 +1395,109 @@ static const struct iio_buffer_setup_ops
> mxs_lradc_buffer_ops = {
>  	.validate_scan_mask = &mxs_lradc_validate_scan_mask,
>  };
>
> +static int mxs_lradc_regulator_is_enabled(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +	int reg = readl(lradc->base + LRADC_CTRL2);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE0;
> +	else if (dev->desc == &lradc->isrc1)
> +		return reg & LRADC_CTRL2_TEMP_SENSOR_IENABLE1;
> +
> +	/* This should never happen */
> +	return -ENODEV;
> +}
> +
> +#define LRADC_REGVALUE2uA(regval, offset) \
> +	(20 * ((regval >> offset) & LRADC_CTRL2_TEMP_ISRC_MASK))
> +
> +static int mxs_lradc_regulator_get_current_limit(struct 
> regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +	int reg = readl(lradc->base + LRADC_CTRL2);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC0_OFFSET);
> +	else if (dev->desc == &lradc->isrc1)
> +		return LRADC_REGVALUE2uA(reg, LRADC_CTRL2_TEMP_ISRC1_OFFSET);
> +
> +	/* This should never happen */
> +	return -ENODEV;
> +}
> +
> +static int mxs_lradc_regulator_enable(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
> +				  LRADC_CTRL2);
> +	else if (dev->desc == &lradc->isrc1)
> +		mxs_lradc_reg_set(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
> +				  LRADC_CTRL2);
> +	else
> +		/* This should never happen */
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int mxs_lradc_regulator_disable(struct regulator_dev *dev)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +
> +	if (dev->desc == &lradc->isrc0)
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0,
> +				    LRADC_CTRL2);
> +	else if (dev->desc == &lradc->isrc1)
> +		mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
> +				    LRADC_CTRL2);
> +	else
> +		/* This should never happen */
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int mxs_lradc_regulator_set_current_limit(struct 
> regulator_dev *dev,
> +						 int min_uA, int max_uA)
> +{
> +	struct mxs_lradc *lradc = rdev_get_drvdata(dev);
> +	int offset, value;
> +
> +	if (dev->desc == &lradc->isrc0)
> +		offset = LRADC_CTRL2_TEMP_ISRC0_OFFSET;
> +	else if (dev->desc == &lradc->isrc1)
> +		offset = LRADC_CTRL2_TEMP_ISRC1_OFFSET;
> +	else
> +		/* This should never happen */
> +		return -ENODEV;
> +
> +	value = min_uA / 20;
> +	if (min_uA % 20)
> +		value++;
> +	if (value * 20 > max_uA)
> +		return -EINVAL;
> +	if (value & ~LRADC_CTRL2_TEMP_ISRC_MASK)
> +		/* This should never happen */
> +		return -EPERM;
> +
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_ISRC_MASK << offset,
> +			    LRADC_CTRL2);
> +	mxs_lradc_reg_set(lradc, value << offset, LRADC_CTRL2);
> +
> +	return 0;
> +}
> +
> +static struct regulator_ops mxs_lradc_regulator_current_ops = {
> +	.enable = mxs_lradc_regulator_enable,
> +	.is_enabled = mxs_lradc_regulator_is_enabled,
> +	.disable = mxs_lradc_regulator_disable,
> +	.get_current_limit = mxs_lradc_regulator_get_current_limit,
> +	.set_current_limit = mxs_lradc_regulator_set_current_limit,
> +};
> +
>  /*
>   * Driver initialization
>   */
> @@ -1519,6 +1634,10 @@ static void mxs_lradc_hw_stop(struct mxs_lradc 
> *lradc)
>
>  	for (i = 0; i < LRADC_MAX_DELAY_CHANS; i++)
>  		mxs_lradc_reg_wrt(lradc, 0, LRADC_DELAY(i));
> +
> +	mxs_lradc_reg_clear(lradc, LRADC_CTRL2_TEMP_SENSOR_IENABLE0 |
> +					LRADC_CTRL2_TEMP_SENSOR_IENABLE1,
> +			    LRADC_CTRL2);
>  }
>
>  static const struct of_device_id mxs_lradc_dt_ids[] = {
> @@ -1592,6 +1711,32 @@ static int mxs_lradc_probe_touchscreen(struct
> mxs_lradc *lradc,
>  	return 0;
>  }
>
> +static void mxs_lradc_reg_helper(struct device_node *np, const char 
> *name,
> +				 struct regulator_config *conf,
> +				 struct regulator_desc *desc)
> +{
> +	struct regulator_dev *ret;
> +
> +	conf->of_node = of_get_child_by_name(np, name);
> +	if (!conf->of_node)
> +		return;
> +
> +	desc->name = name;
> +	desc->owner = THIS_MODULE;
> +	desc->type = REGULATOR_CURRENT;
> +	desc->ops = &mxs_lradc_regulator_current_ops;
> +
> +	conf->init_data = of_get_regulator_init_data(conf->dev, 
> conf->of_node,
> +						     desc);
> +	ret = devm_regulator_register(conf->dev, desc, conf);
> +	if (IS_ERR(ret))
> +		/* Just pretend the regulator isn't there */
> +		dev_err(conf->dev, "Failed to register regulator %s: %ld\n",
> +			desc->name, PTR_ERR(ret));
> +
> +	of_node_put(conf->of_node);
> +}
> +
>  static int mxs_lradc_probe(struct platform_device *pdev)
>  {
>  	const struct of_device_id *of_id =
> @@ -1603,6 +1748,7 @@ static int mxs_lradc_probe(struct
> platform_device *pdev)
>  	struct mxs_lradc *lradc;
>  	struct iio_dev *iio;
>  	struct resource *iores;
> +	struct regulator_config regconf;
>  	int ret = 0, touch_ret;
>  	int i, s;
>  	u64 scale_uv;
> @@ -1727,6 +1873,12 @@ static int mxs_lradc_probe(struct
> platform_device *pdev)
>  		goto err_ts;
>  	}
>
> +	/* Setup regulator devices for current source. */
> +	regconf.dev = dev;
> +	regconf.driver_data = lradc;
> +	mxs_lradc_reg_helper(node, "ISRC0", &regconf, &lradc->isrc0);
> +	mxs_lradc_reg_helper(node, "ISRC1", &regconf, &lradc->isrc1);
> +
>  	return 0;
>
>  err_ts:

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Stefan Wahren]

Hi Harald,

Am 22.04.2016 um 15:52 schrieb Harald Geyer:
> The hardware has two current sources ISRC0 and ISRC1 to allow measuring
> resistors without additional circuitry. This commit makes them available
> as regulators.
>
> Tested on an imx233-olinuxino board.
>
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> ---
> The current regulator API doesn't fit this type of device very well: Typically
> consumers will want to set a defined current, ie. min_uA == max_uA, but they
> can't without help from configuration data, because the valid values aren't
> reported by the API for current regulators. I have been thinking about
> extending the API, but currently AFAIK no such consumers exist and most
> users, like myself, will force the regulator to a defined value in
> devicetree anyway.
>
>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>  drivers/iio/adc/Kconfig                            |   1 +
>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>  3 files changed, 182 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> index 555fb11..983952c 100644
> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> @@ -19,6 +19,15 @@ Optional properties:
>  - fsl,settling: delay between plate switch to next sample. Allowed value is
>                  1 ... 2047. It counts at 2 kHz and its default is
>                  10 (= 5 ms)
> +- ISRC0: A node describing the regulator of internal current source 0
> +- ISRC1: A node describing the regulator of internal current source 1
> +
> +Required properties for the ISRCx sub-nodes:
> +- regulator-max-microamp: See standard regulator binding documentation.
> +                          Valid values are from 0 to 300 in steps of 20.
> +
> +Optional properties for the ISRCx sub-nodes:
> +Any standard regulator properties that apply to current regulators.
>  
>  Example for i.MX23 SoC:
>  
> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>  		fsl,ave-ctrl = <4>;
>  		fsl,ave-delay = <2>;
>  		fsl,settling = <10>;
> +
> +		isrc_lradc0: ISRC0 {
> +			regulator-max-microamp = <300>;
> +		};
> +
> +		isrc_lradc1: ISRC1 {
> +			regulator-max-microamp = <120>;
> +			regulator-min-microamp = <120>;
> +			regulator-always-on;
> +		};

IMHO the binding should represent the direct connection between the
current source and the physical channel. I think a node label isn't
enough. Unfortunately i don't have an idea to do it the "right way".

>  	};
>  
>  Example for i.MX28 SoC:
> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>  		fsl,ave-ctrl = <4>;
>  		fsl,ave-delay = <2>;
>  		fsl,settling = <10>;
> +
> +		isrc_lradc0: ISRC0 {
> +			regulator-max-microamp = <300>;
> +		};
> +
> +		isrc_lradc6: ISRC1 {
> +			regulator-max-microamp = <120>;
> +			regulator-min-microamp = <120>;
> +			regulator-always-on;
> +		};
>  	};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 5937030..1968d1c 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -319,6 +319,7 @@ config MXS_LRADC
>          tristate "Freescale i.MX23/i.MX28 LRADC"
>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>          depends on INPUT
> +        depends on REGULATOR

Any chance to avoid this dependency for an optional feature?

Stefan

>          select STMP_DEVICE
>          select IIO_BUFFER
>          select IIO_TRIGGERED_BUFFER

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Harald Geyer]

Hi Stefan!

On 03.05.2016 13:07, Stefan Wahren wrote:
> Am 22.04.2016 um 15:52 schrieb Harald Geyer:
>> The hardware has two current sources ISRC0 and ISRC1 to allow 
>> measuring
>> resistors without additional circuitry. This commit makes them 
>> available
>> as regulators.
>>
>> Tested on an imx233-olinuxino board.
>>
>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>> ---
>> The current regulator API doesn't fit this type of device very well: 
>> Typically
>> consumers will want to set a defined current, ie. min_uA == max_uA, 
>> but they
>> can't without help from configuration data, because the valid values 
>> aren't
>> reported by the API for current regulators. I have been thinking 
>> about
>> extending the API, but currently AFAIK no such consumers exist and 
>> most
>> users, like myself, will force the regulator to a defined value in
>> devicetree anyway.
>>
>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>  drivers/iio/adc/Kconfig                            |   1 +
>>  drivers/iio/adc/mxs-lradc.c                        | 152 
>> +++++++++++++++++++++
>>  3 files changed, 182 insertions(+)
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt 
>> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>> index 555fb11..983952c 100644
>> --- 
>> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>> +++ 
>> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>> @@ -19,6 +19,15 @@ Optional properties:
>>  - fsl,settling: delay between plate switch to next sample. Allowed 
>> value is
>>                  1 ... 2047. It counts at 2 kHz and its default is
>>                  10 (= 5 ms)
>> +- ISRC0: A node describing the regulator of internal current source 
>> 0
>> +- ISRC1: A node describing the regulator of internal current source 
>> 1
>> +
>> +Required properties for the ISRCx sub-nodes:
>> +- regulator-max-microamp: See standard regulator binding 
>> documentation.
>> +                          Valid values are from 0 to 300 in steps 
>> of 20.
>> +
>> +Optional properties for the ISRCx sub-nodes:
>> +Any standard regulator properties that apply to current regulators.
>>
>>  Example for i.MX23 SoC:
>>
>> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>>  		fsl,ave-ctrl = <4>;
>>  		fsl,ave-delay = <2>;
>>  		fsl,settling = <10>;
>> +
>> +		isrc_lradc0: ISRC0 {
>> +			regulator-max-microamp = <300>;
>> +		};
>> +
>> +		isrc_lradc1: ISRC1 {
>> +			regulator-max-microamp = <120>;
>> +			regulator-min-microamp = <120>;
>> +			regulator-always-on;
>> +		};
>
> IMHO the binding should represent the direct connection between the
> current source and the physical channel. I think a node label isn't
> enough. Unfortunately i don't have an idea to do it the "right way".

I strictly followed the names of the data sheet here: imx23 and imx28
reference manuals both call the current sources ISRC0 and ISRC1 even
if they are attached to different channels of the ADC. I think
inventing some ISRC6 for imx28 would do more harm then good.

>>  	};
>>
>>  Example for i.MX28 SoC:
>> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>>  		fsl,ave-ctrl = <4>;
>>  		fsl,ave-delay = <2>;
>>  		fsl,settling = <10>;
>> +
>> +		isrc_lradc0: ISRC0 {
>> +			regulator-max-microamp = <300>;
>> +		};
>> +
>> +		isrc_lradc6: ISRC1 {
>> +			regulator-max-microamp = <120>;
>> +			regulator-min-microamp = <120>;
>> +			regulator-always-on;
>> +		};
>>  	};
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 5937030..1968d1c 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -319,6 +319,7 @@ config MXS_LRADC
>>          tristate "Freescale i.MX23/i.MX28 LRADC"
>>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>>          depends on INPUT
>> +        depends on REGULATOR
>
> Any chance to avoid this dependency for an optional feature?

I haven't looked into it yet, but I expect that that the mfd
conversion will avoid this neatly.

Thanks,
Harald

> Stefan
>
>>          select STMP_DEVICE
>>          select IIO_BUFFER
>>          select IIO_TRIGGERED_BUFFER

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or bitcoin 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Jonathan Cameron]

On 03/05/16 12:22, Harald Geyer wrote:
> Hi Stefan!
> 
> On 03.05.2016 13:07, Stefan Wahren wrote:
>> Am 22.04.2016 um 15:52 schrieb Harald Geyer:
>>> The hardware has two current sources ISRC0 and ISRC1 to allow measuring
>>> resistors without additional circuitry. This commit makes them available
>>> as regulators.
>>>
>>> Tested on an imx233-olinuxino board.
>>>
>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>>> ---
>>> The current regulator API doesn't fit this type of device very well: Typically
>>> consumers will want to set a defined current, ie. min_uA == max_uA, but they
>>> can't without help from configuration data, because the valid values aren't
>>> reported by the API for current regulators. I have been thinking about
>>> extending the API, but currently AFAIK no such consumers exist and most
>>> users, like myself, will force the regulator to a defined value in
>>> devicetree anyway.
>>>
>>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>>  drivers/iio/adc/Kconfig                            |   1 +
>>>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>>>  3 files changed, 182 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> index 555fb11..983952c 100644
>>> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> @@ -19,6 +19,15 @@ Optional properties:
>>>  - fsl,settling: delay between plate switch to next sample. Allowed value is
>>>                  1 ... 2047. It counts at 2 kHz and its default is
>>>                  10 (= 5 ms)
>>> +- ISRC0: A node describing the regulator of internal current source 0
>>> +- ISRC1: A node describing the regulator of internal current source 1
>>> +
>>> +Required properties for the ISRCx sub-nodes:
>>> +- regulator-max-microamp: See standard regulator binding documentation.
>>> +                          Valid values are from 0 to 300 in steps of 20.
>>> +
>>> +Optional properties for the ISRCx sub-nodes:
>>> +Any standard regulator properties that apply to current regulators.
>>>
>>>  Example for i.MX23 SoC:
>>>
>>> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>>>          fsl,ave-ctrl = <4>;
>>>          fsl,ave-delay = <2>;
>>>          fsl,settling = <10>;
>>> +
>>> +        isrc_lradc0: ISRC0 {
>>> +            regulator-max-microamp = <300>;
>>> +        };
>>> +
>>> +        isrc_lradc1: ISRC1 {
>>> +            regulator-max-microamp = <120>;
>>> +            regulator-min-microamp = <120>;
>>> +            regulator-always-on;
>>> +        };
>>
>> IMHO the binding should represent the direct connection between the
>> current source and the physical channel. I think a node label isn't
>> enough. Unfortunately i don't have an idea to do it the "right way".
> 
> I strictly followed the names of the data sheet here: imx23 and imx28
> reference manuals both call the current sources ISRC0 and ISRC1 even
> if they are attached to different channels of the ADC. I think
> inventing some ISRC6 for imx28 would do more harm then good.
So just to check... These two current sources are effectively
pushing current out of the ADC input pin?  Assumption being that
this causes an appropriate voltage across a component between there
and 0V?

I wonder if we are better off not treating these as regulators at
all - whilst you could in theory use them for general purposes, it
would be a bit nuts and sometimes the 'it is nuts argument' is
enough to mean we don't support something :)

I wouldn't have any problem with an additional attribute for the
adc channels - maybe something like...

in_voltage6_sourcecurrent?  Then the control could be pushed to
userspace. 

Or if it only makes sense to do it as a fixed value in DT then
define optional child nodes for the individual channels, with
the relevant ones have current-source-microamp or
something like that as an parameter.

What do you think?

Jonathan

> 
>>>      };
>>>
>>>  Example for i.MX28 SoC:
>>> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>>>          fsl,ave-ctrl = <4>;
>>>          fsl,ave-delay = <2>;
>>>          fsl,settling = <10>;
>>> +
>>> +        isrc_lradc0: ISRC0 {
>>> +            regulator-max-microamp = <300>;
>>> +        };
>>> +
>>> +        isrc_lradc6: ISRC1 {
>>> +            regulator-max-microamp = <120>;
>>> +            regulator-min-microamp = <120>;
>>> +            regulator-always-on;
>>> +        };
>>>      };
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 5937030..1968d1c 100644
>>> --- a/drivers/iio/adc/Kconfig
>>> +++ b/drivers/iio/adc/Kconfig
>>> @@ -319,6 +319,7 @@ config MXS_LRADC
>>>          tristate "Freescale i.MX23/i.MX28 LRADC"
>>>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>>>          depends on INPUT
>>> +        depends on REGULATOR
>>
>> Any chance to avoid this dependency for an optional feature?
> 
> I haven't looked into it yet, but I expect that that the mfd
> conversion will avoid this neatly.
> 
> Thanks,
> Harald
> 
>> Stefan
>>
>>>          select STMP_DEVICE
>>>          select IIO_BUFFER
>>>          select IIO_TRIGGERED_BUFFER
> 

[PATCH 1/3] iio: mxs-lradc: Add regulators for current sources

[Harald Geyer]

On 04.05.2016 09:15, Jonathan Cameron wrote:
> On 03/05/16 12:22, Harald Geyer wrote:
>> On 03.05.2016 13:07, Stefan Wahren wrote:
>>> Am 22.04.2016 um 15:52 schrieb Harald Geyer:
>>>> The hardware has two current sources ISRC0 and ISRC1 to allow 
>>>> measuring
>>>> resistors without additional circuitry. This commit makes them 
>>>> available
>>>> as regulators.
>>>>
>>>> Tested on an imx233-olinuxino board.
>>>>
>>>> Signed-off-by: Harald Geyer <harald@ccbib.org>
>>>> ---
>>>> The current regulator API doesn't fit this type of device very 
>>>> well: Typically
>>>> consumers will want to set a defined current, ie. min_uA == 
>>>> max_uA, but they
>>>> can't without help from configuration data, because the valid 
>>>> values aren't
>>>> reported by the API for current regulators. I have been thinking 
>>>> about
>>>> extending the API, but currently AFAIK no such consumers exist and 
>>>> most
>>>> users, like myself, will force the regulator to a defined value in
>>>> devicetree anyway.
>>>>
>>>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>>>  drivers/iio/adc/Kconfig                            |   1 +
>>>>  drivers/iio/adc/mxs-lradc.c                        | 152 
>>>> +++++++++++++++++++++
>>>>  3 files changed, 182 insertions(+)
>>>>
>>>> diff --git 
>>>> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt 
>>>> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> index 555fb11..983952c 100644
>>>> --- 
>>>> a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> +++ 
>>>> b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> @@ -19,6 +19,15 @@ Optional properties:
>>>>  - fsl,settling: delay between plate switch to next sample. 
>>>> Allowed value is
>>>>                  1 ... 2047. It counts at 2 kHz and its default is
>>>>                  10 (= 5 ms)
>>>> +- ISRC0: A node describing the regulator of internal current 
>>>> source 0
>>>> +- ISRC1: A node describing the regulator of internal current 
>>>> source 1
>>>> +
>>>> +Required properties for the ISRCx sub-nodes:
>>>> +- regulator-max-microamp: See standard regulator binding 
>>>> documentation.
>>>> +                          Valid values are from 0 to 300 in steps 
>>>> of 20.
>>>> +
>>>> +Optional properties for the ISRCx sub-nodes:
>>>> +Any standard regulator properties that apply to current 
>>>> regulators.
>>>>
>>>>  Example for i.MX23 SoC:
>>>>
>>>> @@ -31,6 +40,16 @@ Example for i.MX23 SoC:
>>>>          fsl,ave-ctrl = <4>;
>>>>          fsl,ave-delay = <2>;
>>>>          fsl,settling = <10>;
>>>> +
>>>> +        isrc_lradc0: ISRC0 {
>>>> +            regulator-max-microamp = <300>;
>>>> +        };
>>>> +
>>>> +        isrc_lradc1: ISRC1 {
>>>> +            regulator-max-microamp = <120>;
>>>> +            regulator-min-microamp = <120>;
>>>> +            regulator-always-on;
>>>> +        };
>>>
>>> IMHO the binding should represent the direct connection between the
>>> current source and the physical channel. I think a node label isn't
>>> enough. Unfortunately i don't have an idea to do it the "right 
>>> way".
>>
>> I strictly followed the names of the data sheet here: imx23 and 
>> imx28
>> reference manuals both call the current sources ISRC0 and ISRC1 even
>> if they are attached to different channels of the ADC. I think
>> inventing some ISRC6 for imx28 would do more harm then good.
> So just to check... These two current sources are effectively
> pushing current out of the ADC input pin?  Assumption being that
> this causes an appropriate voltage across a component between there
> and 0V?

Exactly.

> I wonder if we are better off not treating these as regulators at
> all - whilst you could in theory use them for general purposes, it
> would be a bit nuts and sometimes the 'it is nuts argument' is
> enough to mean we don't support something :)

Yeah, it would be some funny 4-bit DAC ...

> I wouldn't have any problem with an additional attribute for the
> adc channels - maybe something like...
>
> in_voltage6_sourcecurrent?  Then the control could be pushed to
> userspace.

I'm very open to that idea. Not having yet an other subsystem
involved would simplify some things a lot.

> Or if it only makes sense to do it as a fixed value in DT then
> define optional child nodes for the individual channels, with
> the relevant ones have current-source-microamp or
> something like that as an parameter.

My research in thermistors is still ongoing (so I don't have
a kernel driver yet) but I think for thermistors with
exponential temperature dependence dynamic setting of the
current source will be necessary to get high resolution across
the whole temperature range.

Of course many applications wont need this. For checks like
"Is my battery over-heating?" a simple fixed current from DT
will to well, so I guess we should support both.

> What do you think?

As I already stated in an other subthread, I'm not entirely
happy with the regulator approach. Having everything inside
iio would make supporting complex use cases easier in the
future, I guess.

Also IIO devices are much easier to control from user space
then regulators, which might be desired for this type of
device.

OTOH I'm not sure how many other devices out there have a
similar feature. I'd rather not invent a new API for just
very few devices. IMO iio already suffers from the
complexity of the sysfs API. Maybe somebody with a good
overview of ADCs available on the market can provide some
input here.

Also of course there are lots of ADCs out there without
internal current source and those could be used with
thermistors too but would need a regulator anyway.
So maybe eventually we would end up with both solutions
supported, which I'd avoid if at all possible. (I guess
we could work around the last issue by providing some
generic driver to pull an iio channel and a regulator
together - do we like that level of abstraction?)

If we start our own API to control current sources we
might end up pretty much reimplementing the regulator
API for the single consumer case.

I'd like to have input from more people on the above
points before going one way or the other.

Thanks,
Harald

> Jonathan
>
>>
>>>>      };
>>>>
>>>>  Example for i.MX28 SoC:
>>>> @@ -44,4 +63,14 @@ Example for i.MX28 SoC:
>>>>          fsl,ave-ctrl = <4>;
>>>>          fsl,ave-delay = <2>;
>>>>          fsl,settling = <10>;
>>>> +
>>>> +        isrc_lradc0: ISRC0 {
>>>> +            regulator-max-microamp = <300>;
>>>> +        };
>>>> +
>>>> +        isrc_lradc6: ISRC1 {
>>>> +            regulator-max-microamp = <120>;
>>>> +            regulator-min-microamp = <120>;
>>>> +            regulator-always-on;
>>>> +        };
>>>>      };
>>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>>> index 5937030..1968d1c 100644
>>>> --- a/drivers/iio/adc/Kconfig
>>>> +++ b/drivers/iio/adc/Kconfig
>>>> @@ -319,6 +319,7 @@ config MXS_LRADC
>>>>          tristate "Freescale i.MX23/i.MX28 LRADC"
>>>>          depends on (ARCH_MXS || COMPILE_TEST) && HAS_IOMEM
>>>>          depends on INPUT
>>>> +        depends on REGULATOR
>>>
>>> Any chance to avoid this dependency for an optional feature?
>>
>> I haven't looked into it yet, but I expect that that the mfd
>> conversion will avoid this neatly.
>>
>> Thanks,
>> Harald
>>
>>> Stefan
>>>
>>>>          select STMP_DEVICE
>>>>          select IIO_BUFFER
>>>>          select IIO_TRIGGERED_BUFFER
>>

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or bitcoin 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

[PATCH 2/3] ARM: dts: imx23: Provide regulators for the current sources of the LRADC

[Harald Geyer]

This adds the devicetree nodes to make the mxs-lradc driver register
regulator devices for the integrated current sources.

Tested on an imx23-olinuxino board.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
Note: with this configuration the regulator devices will be present but
disabled by default.

 arch/arm/boot/dts/imx23.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
index 302d116..05ff6f1 100644
--- a/arch/arm/boot/dts/imx23.dtsi
+++ b/arch/arm/boot/dts/imx23.dtsi
@@ -471,6 +471,14 @@
 				status = "disabled";
 				clocks = <&clks 26>;
 				#io-channel-cells = <1>;
+
+				isrc_lradc0: ISRC0 {
+					regulator-max-microamp = <300>;
+				};
+
+				isrc_lradc1: ISRC1 {
+					regulator-max-microamp = <300>;
+				};
 			};
 
 			spdif at 80054000 {
-- 
2.1.4

[PATCH 3/3] ARM: dts: imx28: Provide regulators for the current sources of the LRADC

[Harald Geyer]

This adds the devicetree nodes to make the mxs-lradc driver register
regulator devices for the integrated current sources.

Signed-off-by: Harald Geyer <harald@ccbib.org>
---
I couln't test this patch on actual HW, but it is a straight forward
port from the imx23 case. With only a funny change in the node label,
to match the naming convention of the IMX28 Reference Manual.

 arch/arm/boot/dts/imx28.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index f637ec9..38f8675 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -1070,6 +1070,14 @@
 				status = "disabled";
 				clocks = <&clks 41>;
 				#io-channel-cells = <1>;
+
+				isrc_lradc0: ISRC0 {
+					regulator-max-microamp = <300>;
+				};
+
+				isrc_lradc6: ISRC1 {
+					regulator-max-microamp = <300>;
+				};
 			};
 
 			spdif: spdif at 80054000 {
-- 
2.1.4

[PATCH 0/3] mxs-lradc: Add support for current sources

[Stefan Wahren]

Hi Harald,

Am 22.04.2016 um 15:52 schrieb Harald Geyer:
> Patch 1/3 changes the driver and updates the binding documentation
> (I guess it is still in staging.)
>
> Patches 2/3 and 3/3 add the devicetree nodes to imx23 and imx28 boards.
> I'd like to get input whether this is actually desired. On boards where
> these regulators would never be enabled this costs a few extra bytes of
> RAM for allocation of the device data, because the nodes can't be easily
> removed in .dts files which are including the .dtsi files. The alternative
> is to add the new nodes to many .dts files, which would be a lot code
> duplication.

if i get it right the real intention of this patch series is to make the
mxs-lradc provide resistance values instead of voltages. So how about
dropping the whole regulator stuff and provide the values as
IIO_RESISTANCE via iio interface?

Btw this feature should be only added to dts files where is actually used.

Regards
Stefan

>
> Harald Geyer (3):
>   iio: mxs-lradc: Add regulators for current sources
>   ARM: dts: imx23: Provide regulators for the current sources of the
>     LRADC
>   ARM: dts: imx28: Provide regulators for the current sources of the
>     LRADC
>
>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>  arch/arm/boot/dts/imx23.dtsi                       |   8 ++
>  arch/arm/boot/dts/imx28.dtsi                       |   8 ++
>  drivers/iio/adc/Kconfig                            |   1 +
>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>  5 files changed, 198 insertions(+)
>

[PATCH 0/3] mxs-lradc: Add support for current sources

[Harald Geyer]

Hi Stefan!

On 29.04.2016 17:12, Stefan Wahren wrote:
> Hi Harald,
>
> Am 22.04.2016 um 15:52 schrieb Harald Geyer:
>> Patch 1/3 changes the driver and updates the binding documentation
>> (I guess it is still in staging.)
>>
>> Patches 2/3 and 3/3 add the devicetree nodes to imx23 and imx28 
>> boards.
>> I'd like to get input whether this is actually desired. On boards 
>> where
>> these regulators would never be enabled this costs a few extra bytes 
>> of
>> RAM for allocation of the device data, because the nodes can't be 
>> easily
>> removed in .dts files which are including the .dtsi files. The 
>> alternative
>> is to add the new nodes to many .dts files, which would be a lot 
>> code
>> duplication.
>
> if i get it right the real intention of this patch series is to make 
> the
> mxs-lradc provide resistance values instead of voltages.

... or even temperature values if a thermistor is connected.

> So how about
> dropping the whole regulator stuff and provide the values as
> IIO_RESISTANCE via iio interface?

Can you elaborate on this?

My thinking is that there should be a thermistor (or whatever else) 
driver,
that is a consumer of the regulator and a consumer of the IIO_VOLTAGE
iio channel and provides a new device with an IIO_RESISTANCE or 
IIO_TEMP
channel. Maybe there is a simpler solution, that I'm missing?

Actually I'm not 100% happy with the above solution myself, because if 
we
start supporting devices that act as an iio-multiplexer (some device 
that
is an iio consumer and provides many new iio channels and can control
via gpios which of it child channels is actually routed to the upstream
device) I don't know how to properly manage the regulator device.
However since this is only hypothetical ATM, I think we don't have to
worry about this too much.

> Btw this feature should be only added to dts files where is actually 
> used.

Ok, so how do we figure out which boards these are?
I use this on the olinuxino and I guess all evaluation boards support 
the
use case too. The others I don't know enough about to be sure and I 
guess
it's not a good idea to just wait until somebody speaks up and 
complains
that the feature is missing on board X ...

Thanks,
Harald

> Regards
> Stefan
>
>>
>> Harald Geyer (3):
>>   iio: mxs-lradc: Add regulators for current sources
>>   ARM: dts: imx23: Provide regulators for the current sources of the
>>     LRADC
>>   ARM: dts: imx28: Provide regulators for the current sources of the
>>     LRADC
>>
>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>  arch/arm/boot/dts/imx23.dtsi                       |   8 ++
>>  arch/arm/boot/dts/imx28.dtsi                       |   8 ++
>>  drivers/iio/adc/Kconfig                            |   1 +
>>  drivers/iio/adc/mxs-lradc.c                        | 152 
>> +++++++++++++++++++++
>>  5 files changed, 198 insertions(+)
>>

-- 
If you want to support my work:
see http://friends.ccbib.org/harald/supporting/
or donate via peercoin to P98LRdhit3gZbHDBe7ta5jtXrMJUms4p7w
or bitcoin 1FUtd8T9jRN1rFz63vZz7s2fDtB6d6A7aS

[PATCH 0/3] mxs-lradc: Add support for current sources

[Jonathan Cameron]

On 29/04/16 18:45, Harald Geyer wrote:
> Hi Stefan!
> 
> On 29.04.2016 17:12, Stefan Wahren wrote:
>> Hi Harald,
>>
>> Am 22.04.2016 um 15:52 schrieb Harald Geyer:
>>> Patch 1/3 changes the driver and updates the binding documentation
>>> (I guess it is still in staging.)
>>>
>>> Patches 2/3 and 3/3 add the devicetree nodes to imx23 and imx28 boards.
>>> I'd like to get input whether this is actually desired. On boards where
>>> these regulators would never be enabled this costs a few extra bytes of
>>> RAM for allocation of the device data, because the nodes can't be easily
>>> removed in .dts files which are including the .dtsi files. The alternative
>>> is to add the new nodes to many .dts files, which would be a lot code
>>> duplication.
>>
>> if i get it right the real intention of this patch series is to make the
>> mxs-lradc provide resistance values instead of voltages.
> 
> ... or even temperature values if a thermistor is connected.
> 
>> So how about
>> dropping the whole regulator stuff and provide the values as
>> IIO_RESISTANCE via iio interface?
> 
> Can you elaborate on this?
> 
> My thinking is that there should be a thermistor (or whatever else) driver,
> that is a consumer of the regulator and a consumer of the IIO_VOLTAGE
> iio channel and provides a new device with an IIO_RESISTANCE or IIO_TEMP
> channel. Maybe there is a simpler solution, that I'm missing?
I agree with that approach - need a way to describe the upstream hardware - so
will need a thermistor driver
> 
> Actually I'm not 100% happy with the above solution myself, because if we
> start supporting devices that act as an iio-multiplexer (some device that
> is an iio consumer and provides many new iio channels and can control
> via gpios which of it child channels is actually routed to the upstream
> device) I don't know how to properly manage the regulator device.
Hmm. Are you talking about muxing the regulator as well?  That will get
complex indeed.  Might be possible to cheat and imply the regulators are
always connected to all inputs...  or do you want to dynamically change
the regulator output?  That gets messy fast - though in theory might be
possible...
> However since this is only hypothetical ATM, I think we don't have to
> worry about this too much.
> 
>> Btw this feature should be only added to dts files where is actually used.
> 
> Ok, so how do we figure out which boards these are?
> I use this on the olinuxino and I guess all evaluation boards support the
> use case too. The others I don't know enough about to be sure and I guess
> it's not a good idea to just wait until somebody speaks up and complains
> that the feature is missing on board X ...
> 
> Thanks,
> Harald
> 
>> Regards
>> Stefan
>>
>>>
>>> Harald Geyer (3):
>>>   iio: mxs-lradc: Add regulators for current sources
>>>   ARM: dts: imx23: Provide regulators for the current sources of the
>>>     LRADC
>>>   ARM: dts: imx28: Provide regulators for the current sources of the
>>>     LRADC
>>>
>>>  .../bindings/staging/iio/adc/mxs-lradc.txt         |  29 ++++
>>>  arch/arm/boot/dts/imx23.dtsi                       |   8 ++
>>>  arch/arm/boot/dts/imx28.dtsi                       |   8 ++
>>>  drivers/iio/adc/Kconfig                            |   1 +
>>>  drivers/iio/adc/mxs-lradc.c                        | 152 +++++++++++++++++++++
>>>  5 files changed, 198 insertions(+)
>>>
> 

[PATCH 0/3] mxs-lradc: Add support for current sources

[Harald Geyer]

On 01.05.2016 20:02, Jonathan Cameron wrote:
> On 29/04/16 18:45, Harald Geyer wrote:
>> My thinking is that there should be a thermistor (or whatever else) 
>> driver,
>> that is a consumer of the regulator and a consumer of the 
>> IIO_VOLTAGE
>> iio channel and provides a new device with an IIO_RESISTANCE or 
>> IIO_TEMP
>> channel. Maybe there is a simpler solution, that I'm missing?
> I agree with that approach - need a way to describe the upstream
> hardware - so
> will need a thermistor driver
>>
>> Actually I'm not 100% happy with the above solution myself, because 
>> if we
>> start supporting devices that act as an iio-multiplexer (some device 
>> that
>> is an iio consumer and provides many new iio channels and can 
>> control
>> via gpios which of it child channels is actually routed to the 
>> upstream
>> device) I don't know how to properly manage the regulator device.
> Hmm. Are you talking about muxing the regulator as well?  That will 
> get
> complex indeed.  Might be possible to cheat and imply the regulators 
> are
> always connected to all inputs...  or do you want to dynamically 
> change
> the regulator output?  That gets messy fast - though in theory might 
> be
> possible...

Yes, that's a good summary of what I'm thinking. As there is no
iio-multiplexer yet, this is sort of an academic question. But it's
kind of an obvious thing to want, so we shouldn't be surprised when
somebody actually starts working on that.

Thanks,
Harald

[PATCH 0/3] mxs-lradc: Add support for current sources

[Stefan Wahren]

Hi Harald,

Am 01.05.2016 um 20:02 schrieb Jonathan Cameron:
> On 29/04/16 18:45, Harald Geyer wrote:
>> Hi Stefan!
>>
>> On 29.04.2016 17:12, Stefan Wahren wrote:
>>> Hi Harald,
>>>
>>> Am 22.04.2016 um 15:52 schrieb Harald Geyer:
>>>> Patch 1/3 changes the driver and updates the binding documentation
>>>> (I guess it is still in staging.)
>>>>
>>>> Patches 2/3 and 3/3 add the devicetree nodes to imx23 and imx28 boards.
>>>> I'd like to get input whether this is actually desired. On boards where
>>>> these regulators would never be enabled this costs a few extra bytes of
>>>> RAM for allocation of the device data, because the nodes can't be easily
>>>> removed in .dts files which are including the .dtsi files. The alternative
>>>> is to add the new nodes to many .dts files, which would be a lot code
>>>> duplication.
>>> if i get it right the real intention of this patch series is to make the
>>> mxs-lradc provide resistance values instead of voltages.
>> ... or even temperature values if a thermistor is connected.
>>
>>> So how about
>>> dropping the whole regulator stuff and provide the values as
>>> IIO_RESISTANCE via iio interface?
>> Can you elaborate on this?
>>
>> My thinking is that there should be a thermistor (or whatever else) driver,
>> that is a consumer of the regulator and a consumer of the IIO_VOLTAGE
>> iio channel and provides a new device with an IIO_RESISTANCE or IIO_TEMP
>> channel. Maybe there is a simpler solution, that I'm missing?
> I agree with that approach - need a way to describe the upstream hardware - so
> will need a thermistor driver

okay, this makes more sense to me. In this case it would be good to have
this new driver in this patch series. So it's possible to verify the
matching binding.

>> Actually I'm not 100% happy with the above solution myself, because if we
>> start supporting devices that act as an iio-multiplexer (some device that
>> is an iio consumer and provides many new iio channels and can control
>> via gpios which of it child channels is actually routed to the upstream
>> device) I don't know how to properly manage the regulator device.
> Hmm. Are you talking about muxing the regulator as well?  That will get
> complex indeed.  Might be possible to cheat and imply the regulators are
> always connected to all inputs...  or do you want to dynamically change
> the regulator output?  That gets messy fast - though in theory might be
> possible...
>> However since this is only hypothetical ATM, I think we don't have to
>> worry about this too much.
>>
>>> Btw this feature should be only added to dts files where is actually used.
>> Ok, so how do we figure out which boards these are?
>> I use this on the olinuxino and I guess all evaluation boards support the
>> use case too. The others I don't know enough about to be sure and I guess
>> it's not a good idea to just wait until somebody speaks up and complains
>> that the feature is missing on board X ...

The imx23 dtsi file specifies features on SoC level and i think this
should be kept. We shouldn't implement features for ADC pins without
deeper knowledge about these boards.

Maybe we could introduce a new dtsi file in order to reduce copy &
paste. But at first the OlinuXino should be a good starting point ...

Regards
Stefan