Greetings,
This is v3 of the patchset that adds support to the optional
divider_by_two of LRADC channels.
Changes in v3:
- Fix merge conflict with previous existing patch.
- Fix ampersand in function callback assignment.
- Removed unused parameters of function to read single sample.
- Ennumeration for two-dimensional array of scale (with divider
by two disabled or enabled), for improved readability.
- Created struct with integer and nano parts of the scale
attribute, for improved readability.
Changes in v2:
- Fix the sample mask passed by the touchscreen driver to the input
subsytem, to be 12 bits.
- Move the reference voltages to the Device Tree.
- Rebased to avoid conflict with Marek Vasut's prior patch.
- Use IIO_DEVICE_ATTR() macro for adding scale_available property to
all channels.
- Make 'is_divided' unsigned int.
Notes:
- Other fixes were discussed in v1 but they were not part of this
patchset changes, and should be handled in a separate patch.
- The 64bit math to calculate the integer and decimal parts of the
scaling attribute is a bit unreadable but used in other similar
drivers like ad7791, ad7793, and ad7192. If it is to be changed
it should be done in parallel with these, in a different patch.
The first patch changes the realbits to 12. The
second adds the channels reference voltages to the DT.
The following add the scale read operation, scale_available read
operation, and scale write operation.
This was tested on a custom i.MX28 platform.
Could someone please test on an i.MX23?
Hector Palacios (5):
iio: mxs-lradc: change the realbits to 12
ARM: dts: add reference voltage property for MXS LRADC
iio: mxs-lradc: add scale attribute to channels
iio: mxs-lradc: add scale_available file to channels
iio: mxs-lradc: add write_raw function to modify scale
.../bindings/staging/iio/adc/mxs-lradc.txt | 9 +-
arch/arm/boot/dts/imx23.dtsi | 4 +
arch/arm/boot/dts/imx28.dtsi | 4 +
drivers/staging/iio/adc/mxs-lradc.c | 255 ++++++++++++++++++---
4 files changed, 242 insertions(+), 30 deletions(-)
Some LRADC channels have fixed pre-dividers so they can measure
different voltages at full scale. The reference voltage allows to
expose a scaling attribute through the IIO sysfs so that a user can
compute the real voltage out of a measured sample value.
Signed-off-by: Hector Palacios <[email protected]>
Acked-by: Marek Vasut <[email protected]>
Acked-by: Alexandre Belloni <[email protected]>
---
Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt | 9 ++++++++-
arch/arm/boot/dts/imx23.dtsi | 4 ++++
arch/arm/boot/dts/imx28.dtsi | 4 ++++
3 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
index 4688205..6ec485c 100644
--- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
+++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
@@ -1,9 +1,12 @@
* Freescale i.MX28 LRADC device driver
Required properties:
-- compatible: Should be "fsl,imx28-lradc"
+- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
- reg: Address and length of the register set for the device
- interrupts: Should contain the LRADC interrupts
+- fsl,vref: Reference voltage (in mV) for each LRADC channel. This is the
+ maximum voltage that can be measured at full scale in each channel
+ considering fixed pre-dividers.
Optional properties:
- fsl,lradc-touchscreen-wires: Number of wires used to connect the touchscreen
@@ -18,4 +21,8 @@ Examples:
reg = <0x80050000 0x2000>;
interrupts = <10 14 15 16 17 18 19
20 21 22 23 24 25>;
+ fsl,vref = <1850 1850 1850 1850
+ 1850 1850 1850 7400
+ 1850 1850 3700 1850
+ 3700 1850 1850 7400>
};
diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
index 587ceef..e212902 100644
--- a/arch/arm/boot/dts/imx23.dtsi
+++ b/arch/arm/boot/dts/imx23.dtsi
@@ -430,6 +430,10 @@
compatible = "fsl,imx23-lradc";
reg = <0x80050000 0x2000>;
interrupts = <36 37 38 39 40 41 42 43 44>;
+ fsl,vref = <1850 1850 1850 1850
+ 1850 1850 3700 7400
+ 1850 1850 1850 1850
+ 1850 1850 1850 7400>;
status = "disabled";
};
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 6a8acb0..c1b3724 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -865,6 +865,10 @@
reg = <0x80050000 0x2000>;
interrupts = <10 14 15 16 17 18 19
20 21 22 23 24 25>;
+ fsl,vref = <1850 1850 1850 1850
+ 1850 1850 1850 7400
+ 1850 1850 3700 1850
+ 3700 1850 1850 7400>;
status = "disabled";
};
The LRADC virtual channels have an 18 bit field to store the sum of up
to 2^5 accumulated samples. The read_raw function however only operates
over a single sample (12 bit resolution).
In order to use this field for scaling operations, we need it to be the
exact resolution value of the LRADC.
Besides, the driver was using an 18 bit mask (LRADC_CH_VALUE_MASK) to
report touch coordinates to userland. A 12 bit mask should be used instead
or else the touch libraries will expect a coordinates range between 0
and 0x3ffff (18 bits), instead of between 0 and 0xfff (12 bits).
Signed-off-by: Hector Palacios <[email protected]>
Acked-by: Marek Vasut <[email protected]>
Acked-by: Alexandre Belloni <[email protected]>
---
drivers/staging/iio/adc/mxs-lradc.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 9f52a28..56667da 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -225,6 +225,9 @@ struct mxs_lradc {
#define LRADC_CTRL4_LRADCSELECT_MASK(n) (0xf << ((n) * 4))
#define LRADC_CTRL4_LRADCSELECT_OFFSET(n) ((n) * 4)
+#define LRADC_RESOLUTION 12
+#define LRADC_SINGLE_SAMPLE_MASK ((1 << LRADC_RESOLUTION) - 1)
+
/*
* Raw I/O operations
*/
@@ -540,9 +543,10 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
__set_bit(EV_ABS, input->evbit);
__set_bit(EV_KEY, input->evbit);
__set_bit(BTN_TOUCH, input->keybit);
- input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
- input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
- input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0, 0);
+ input_set_abs_params(input, ABS_X, 0, LRADC_SINGLE_SAMPLE_MASK, 0, 0);
+ input_set_abs_params(input, ABS_Y, 0, LRADC_SINGLE_SAMPLE_MASK, 0, 0);
+ input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_SINGLE_SAMPLE_MASK,
+ 0, 0);
lradc->ts_input = input;
input_set_drvdata(input, lradc);
@@ -817,7 +821,7 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
.channel = (idx), \
.scan_type = { \
.sign = 'u', \
- .realbits = 18, \
+ .realbits = LRADC_RESOLUTION, \
.storagebits = 32, \
}, \
}
Added write_raw function to manipulate the optional divider_by_two
through the scaling attribute out of the available scales.
Signed-off-by: Hector Palacios <[email protected]>
---
drivers/staging/iio/adc/mxs-lradc.c | 56 ++++++++++++++++++++++++++++++++++++-
1 file changed, 55 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 5d332b1..4d3c7f5 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -213,6 +213,7 @@ struct mxs_lradc {
#define LRADC_CTRL1_LRADC_IRQ_OFFSET 0
#define LRADC_CTRL2 0x20
+#define LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET 24
#define LRADC_CTRL2_TEMPSENSE_PWD (1 << 15)
#define LRADC_STATUS 0x40
@@ -320,7 +321,8 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
break;
case IIO_CHAN_INFO_SCALE:
*val = lradc->vref_mv[chan->channel];
- *val2 = chan->scan_type.realbits;
+ *val2 = chan->scan_type.realbits -
+ lradc->is_divided[chan->channel];
ret = IIO_VAL_FRACTIONAL_LOG2;
break;
default:
@@ -333,6 +335,56 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
return ret;
}
+static int mxs_lradc_write_raw(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ int val, int val2, long m)
+{
+ struct mxs_lradc *lradc = iio_priv(iio_dev);
+ struct mxs_lradc_scale *scale_avail =
+ lradc->scale_avail[chan->channel];
+ int ret;
+
+ ret = mutex_trylock(&lradc->lock);
+ if (!ret)
+ return -EBUSY;
+
+ switch (m) {
+ case IIO_CHAN_INFO_SCALE:
+ ret = -EINVAL;
+ if (val == scale_avail[MXS_LRADC_DIV_DISABLED].integer &&
+ val2 == scale_avail[MXS_LRADC_DIV_DISABLED].nano) {
+ /* divider by two disabled */
+ writel(1 << LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET,
+ lradc->base + LRADC_CTRL2 + STMP_OFFSET_REG_CLR);
+ lradc->is_divided[chan->channel] = 0;
+ ret = 0;
+ } else if (val == scale_avail[MXS_LRADC_DIV_ENABLED].integer &&
+ val2 == scale_avail[MXS_LRADC_DIV_ENABLED].nano) {
+ /* divider by two enabled */
+ writel(1 << LRADC_CTRL2_DIVIDE_BY_TWO_OFFSET,
+ lradc->base + LRADC_CTRL2 + STMP_OFFSET_REG_SET);
+ lradc->is_divided[chan->channel] = 1;
+ ret = 0;
+ }
+
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ mutex_unlock(&lradc->lock);
+
+ return ret;
+}
+
+static int mxs_lradc_write_raw_get_fmt(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ long m)
+{
+ return IIO_VAL_INT_PLUS_NANO;
+}
+
static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
struct device_attribute *attr,
char *buf,
@@ -410,6 +462,8 @@ static const struct attribute_group mxs_lradc_attribute_group = {
static const struct iio_info mxs_lradc_iio_info = {
.driver_module = THIS_MODULE,
.read_raw = mxs_lradc_read_raw,
+ .write_raw = mxs_lradc_write_raw,
+ .write_raw_get_fmt = mxs_lradc_write_raw_get_fmt,
.attrs = &mxs_lradc_attribute_group,
};
Some LRADC channels have fixed pre-dividers and all have an optional
divider by two which allows a maximum input voltage of VDDIO - 50mV.
This patch
- adds the scaling info flag to all channels
- grabs the max reference voltage per channel from DT
(where the fixed pre-dividers apply)
- allows to read the scaling attribute (computed from the Vref)
Signed-off-by: Hector Palacios <[email protected]>.
Acked-by: Marek Vasut <[email protected]>
---
drivers/staging/iio/adc/mxs-lradc.c | 77 +++++++++++++++++++++++++------------
1 file changed, 53 insertions(+), 24 deletions(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 56667da..5037577 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -141,6 +141,8 @@ struct mxs_lradc {
struct completion completion;
+ uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
+
/*
* Touchscreen LRADC channels receives a private slot in the CTRL4
* register, the slot #7. Therefore only 7 slots instead of 8 in the
@@ -228,33 +230,12 @@ struct mxs_lradc {
#define LRADC_RESOLUTION 12
#define LRADC_SINGLE_SAMPLE_MASK ((1 << LRADC_RESOLUTION) - 1)
-/*
- * Raw I/O operations
- */
-static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
- const struct iio_chan_spec *chan,
- int *val, int *val2, long m)
+static int mxs_lradc_read_single(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan, int *val)
{
struct mxs_lradc *lradc = iio_priv(iio_dev);
int ret;
- if (m != IIO_CHAN_INFO_RAW)
- return -EINVAL;
-
- /* Check for invalid channel */
- if (chan->channel > LRADC_MAX_TOTAL_CHANS)
- return -EINVAL;
-
- /*
- * See if there is no buffered operation in progess. If there is, simply
- * bail out. This can be improved to support both buffered and raw IO at
- * the same time, yet the code becomes horribly complicated. Therefore I
- * applied KISS principle here.
- */
- ret = mutex_trylock(&lradc->lock);
- if (!ret)
- return -EBUSY;
-
INIT_COMPLETION(lradc->completion);
/*
@@ -293,6 +274,47 @@ err:
writel(LRADC_CTRL1_LRADC_IRQ_EN(0),
lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
+ return ret;
+}
+
+/*
+ * Raw I/O operations
+ */
+static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long m)
+{
+ struct mxs_lradc *lradc = iio_priv(iio_dev);
+ int ret;
+
+ /*
+ * See if there is no buffered operation in progress. If there is, simply
+ * bail out. This can be improved to support both buffered and raw IO at
+ * the same time, yet the code becomes horribly complicated. Therefore I
+ * applied KISS principle here.
+ */
+ ret = mutex_trylock(&lradc->lock);
+ if (!ret)
+ return -EBUSY;
+
+ /* Check for invalid channel */
+ if (chan->channel > LRADC_MAX_TOTAL_CHANS)
+ ret = -EINVAL;
+
+ switch (m) {
+ case IIO_CHAN_INFO_RAW:
+ ret = mxs_lradc_read_single(iio_dev, chan, val);
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ *val = lradc->vref_mv[chan->channel];
+ *val2 = chan->scan_type.realbits;
+ ret = IIO_VAL_FRACTIONAL_LOG2;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
mutex_unlock(&lradc->lock);
return ret;
@@ -817,7 +839,8 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
.type = (chan_type), \
.indexed = 1, \
.scan_index = (idx), \
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ BIT(IIO_CHAN_INFO_SCALE), \
.channel = (idx), \
.scan_type = { \
.sign = 'u', \
@@ -956,6 +979,12 @@ static int mxs_lradc_probe(struct platform_device *pdev)
goto err_addr;
}
+ /* Grab Vref array from DT */
+ ret = of_property_read_u32_array(node, "fsl,vref", lradc->vref_mv,
+ LRADC_MAX_TOTAL_CHANS);
+ if (ret)
+ goto err_addr;
+
platform_set_drvdata(pdev, iio);
init_completion(&lradc->completion);
Adds in_voltageX_scale_available file for every channel to read
the different available scales.
There are two scales per channel:
[0] = divider_by_two disabled (default)
[1] = divider_by_two enabled
The scale is a struct made of integer and nano parts to build
a long decimal number.
Signed-off-by: Hector Palacios <[email protected]>
---
drivers/staging/iio/adc/mxs-lradc.c | 112 +++++++++++++++++++++++++++++++++++-
1 file changed, 111 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 5037577..5d332b1 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -37,6 +37,7 @@
#include <linux/input.h>
#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
#include <linux/iio/buffer.h>
#include <linux/iio/trigger.h>
#include <linux/iio/trigger_consumer.h>
@@ -129,6 +130,16 @@ enum mxs_lradc_ts {
MXS_LRADC_TOUCHSCREEN_5WIRE,
};
+enum mxs_lradc_divbytwo {
+ MXS_LRADC_DIV_DISABLED = 0,
+ MXS_LRADC_DIV_ENABLED,
+};
+
+struct mxs_lradc_scale {
+ unsigned int integer;
+ unsigned int nano;
+};
+
struct mxs_lradc {
struct device *dev;
void __iomem *base;
@@ -142,6 +153,8 @@ struct mxs_lradc {
struct completion completion;
uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
+ struct mxs_lradc_scale scale_avail[LRADC_MAX_TOTAL_CHANS][2];
+ unsigned int is_divided[LRADC_MAX_TOTAL_CHANS];
/*
* Touchscreen LRADC channels receives a private slot in the CTRL4
@@ -320,9 +333,84 @@ static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
return ret;
}
+static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
+ struct device_attribute *attr,
+ char *buf,
+ int ch)
+{
+ struct iio_dev *iio = dev_to_iio_dev(dev);
+ struct mxs_lradc *lradc = iio_priv(iio);
+ int i, len = 0;
+
+ for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
+ len += sprintf(buf + len, "%d.%09u ",
+ lradc->scale_avail[ch][i].integer,
+ lradc->scale_avail[ch][i].nano);
+
+ len += sprintf(buf + len, "\n");
+
+ return len;
+}
+
+static ssize_t mxs_lradc_show_scale_available(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+
+ return mxs_lradc_show_scale_available_ch(dev, attr, buf,
+ iio_attr->address);
+}
+
+#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
+static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO, \
+ mxs_lradc_show_scale_available, NULL, ch)
+
+SHOW_SCALE_AVAILABLE_ATTR(0);
+SHOW_SCALE_AVAILABLE_ATTR(1);
+SHOW_SCALE_AVAILABLE_ATTR(2);
+SHOW_SCALE_AVAILABLE_ATTR(3);
+SHOW_SCALE_AVAILABLE_ATTR(4);
+SHOW_SCALE_AVAILABLE_ATTR(5);
+SHOW_SCALE_AVAILABLE_ATTR(6);
+SHOW_SCALE_AVAILABLE_ATTR(7);
+SHOW_SCALE_AVAILABLE_ATTR(8);
+SHOW_SCALE_AVAILABLE_ATTR(9);
+SHOW_SCALE_AVAILABLE_ATTR(10);
+SHOW_SCALE_AVAILABLE_ATTR(11);
+SHOW_SCALE_AVAILABLE_ATTR(12);
+SHOW_SCALE_AVAILABLE_ATTR(13);
+SHOW_SCALE_AVAILABLE_ATTR(14);
+SHOW_SCALE_AVAILABLE_ATTR(15);
+
+static struct attribute *mxs_lradc_attributes[] = {
+ &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
+ &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group mxs_lradc_attribute_group = {
+ .attrs = mxs_lradc_attributes,
+};
+
static const struct iio_info mxs_lradc_iio_info = {
.driver_module = THIS_MODULE,
.read_raw = mxs_lradc_read_raw,
+ .attrs = &mxs_lradc_attribute_group,
};
/*
@@ -842,6 +930,7 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
BIT(IIO_CHAN_INFO_SCALE), \
.channel = (idx), \
+ .address = (idx), \
.scan_type = { \
.sign = 'u', \
.realbits = LRADC_RESOLUTION, \
@@ -929,7 +1018,8 @@ static int mxs_lradc_probe(struct platform_device *pdev)
struct resource *iores;
uint32_t ts_wires = 0;
int ret = 0;
- int i;
+ int i, s;
+ unsigned int scale_uv;
/* Allocate the IIO device. */
iio = iio_device_alloc(sizeof(*lradc));
@@ -1008,6 +1098,26 @@ static int mxs_lradc_probe(struct platform_device *pdev)
if (ret)
goto err_trig;
+ /* Populate available ADC input ranges */
+ for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
+ for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
+ /*
+ * [s=0] = optional divider by two disabled (default)
+ * [s=1] = optional divider by two enabled
+ *
+ * The scale is calculated by doing:
+ * Vref >> (realbits - s)
+ * which multiplies by two on the second component
+ * of the array.
+ */
+ scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
+ (iio->channels[i].scan_type.realbits - s);
+ lradc->scale_avail[i][s].nano =
+ do_div(scale_uv, 100000000) * 10;
+ lradc->scale_avail[i][s].integer = scale_uv;
+ }
+ }
+
/* Configure the hardware. */
mxs_lradc_hw_init(lradc);
On 07/22/2013 04:04 PM, Hector Palacios wrote:
> Some LRADC channels have fixed pre-dividers so they can measure
> different voltages at full scale. The reference voltage allows to
> expose a scaling attribute through the IIO sysfs so that a user can
> compute the real voltage out of a measured sample value.
I've said before that I'm not convinced that this is the right way to implement
this. And considering what Thomas said here
http://www.mail-archive.com/[email protected]/msg36691.html I
guess I'm not alone with that opinion.
- Lars
>
> Signed-off-by: Hector Palacios <[email protected]>
> Acked-by: Marek Vasut <[email protected]>
> Acked-by: Alexandre Belloni <[email protected]>
> ---
> Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt | 9 ++++++++-
> arch/arm/boot/dts/imx23.dtsi | 4 ++++
> arch/arm/boot/dts/imx28.dtsi | 4 ++++
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> index 4688205..6ec485c 100644
> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> @@ -1,9 +1,12 @@
> * Freescale i.MX28 LRADC device driver
>
> Required properties:
> -- compatible: Should be "fsl,imx28-lradc"
> +- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
> - reg: Address and length of the register set for the device
> - interrupts: Should contain the LRADC interrupts
> +- fsl,vref: Reference voltage (in mV) for each LRADC channel. This is the
> + maximum voltage that can be measured at full scale in each channel
> + considering fixed pre-dividers.
>
> Optional properties:
> - fsl,lradc-touchscreen-wires: Number of wires used to connect the touchscreen
> @@ -18,4 +21,8 @@ Examples:
> reg = <0x80050000 0x2000>;
> interrupts = <10 14 15 16 17 18 19
> 20 21 22 23 24 25>;
> + fsl,vref = <1850 1850 1850 1850
> + 1850 1850 1850 7400
> + 1850 1850 3700 1850
> + 3700 1850 1850 7400>
> };
> diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
> index 587ceef..e212902 100644
> --- a/arch/arm/boot/dts/imx23.dtsi
> +++ b/arch/arm/boot/dts/imx23.dtsi
> @@ -430,6 +430,10 @@
> compatible = "fsl,imx23-lradc";
> reg = <0x80050000 0x2000>;
> interrupts = <36 37 38 39 40 41 42 43 44>;
> + fsl,vref = <1850 1850 1850 1850
> + 1850 1850 3700 7400
> + 1850 1850 1850 1850
> + 1850 1850 1850 7400>;
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> index 6a8acb0..c1b3724 100644
> --- a/arch/arm/boot/dts/imx28.dtsi
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -865,6 +865,10 @@
> reg = <0x80050000 0x2000>;
> interrupts = <10 14 15 16 17 18 19
> 20 21 22 23 24 25>;
> + fsl,vref = <1850 1850 1850 1850
> + 1850 1850 1850 7400
> + 1850 1850 3700 1850
> + 3700 1850 1850 7400>;
> status = "disabled";
> };
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Dear Hector Palacios,
> Adds in_voltageX_scale_available file for every channel to read
> the different available scales.
> There are two scales per channel:
> [0] = divider_by_two disabled (default)
> [1] = divider_by_two enabled
> The scale is a struct made of integer and nano parts to build
> a long decimal number.
>
> Signed-off-by: Hector Palacios <[email protected]>
> ---
[...]
> @@ -1008,6 +1098,26 @@ static int mxs_lradc_probe(struct platform_device
> *pdev) if (ret)
> goto err_trig;
>
> + /* Populate available ADC input ranges */
> + for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
> + for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
> + /*
> + * [s=0] = optional divider by two disabled (default)
> + * [s=1] = optional divider by two enabled
> + *
> + * The scale is calculated by doing:
> + * Vref >> (realbits - s)
> + * which multiplies by two on the second component
> + * of the array.
> + */
> + scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
> + (iio->channels[i].scan_type.realbits - s);
Thinking about this, this is basically
vref_mv[CHANNEL] * 100 000 000
scale_uv = --------------------------------
2^(12 - s)
Where s can be either 0 or 1.
Why do you multiply it by 100000000 I don't quite understand, but maybe it's
fully obvious. It should be documented though.
> + lradc->scale_avail[i][s].nano =
> + do_div(scale_uv, 100000000) * 10;
Are we not loosing precission here?
> + lradc->scale_avail[i][s].integer = scale_uv;
> + }
> + }
> +
> /* Configure the hardware. */
> mxs_lradc_hw_init(lradc);
Best regards,
Marek Vasut
Dear Lars-Peter Clausen,
> On 07/22/2013 04:04 PM, Hector Palacios wrote:
> > Some LRADC channels have fixed pre-dividers so they can measure
> > different voltages at full scale. The reference voltage allows to
> > expose a scaling attribute through the IIO sysfs so that a user can
> > compute the real voltage out of a measured sample value.
>
> I've said before that I'm not convinced that this is the right way to
> implement this. And considering what Thomas said here
> http://www.mail-archive.com/[email protected]/msg36691.ht
> ml I guess I'm not alone with that opinion.
>
> - Lars
He's talking about different versions of the IP block, this is the same IP
block, just connected to different inputs.
Best regards,
Marek Vasut
Dear Hector Palacios,
> Added write_raw function to manipulate the optional divider_by_two
> through the scaling attribute out of the available scales.
>
> Signed-off-by: Hector Palacios <[email protected]>
> ---
Yep, looks nicer,
Reviewed-by: Marek Vasut <[email protected]>
Best regards,
Marek Vasut
Hello Marek,
On 07/23/2013 12:36 AM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Adds in_voltageX_scale_available file for every channel to read
>> the different available scales.
>> There are two scales per channel:
>> [0] = divider_by_two disabled (default)
>> [1] = divider_by_two enabled
>> The scale is a struct made of integer and nano parts to build
>> a long decimal number.
>>
>> Signed-off-by: Hector Palacios <[email protected]>
>> ---
>
> [...]
>
>> @@ -1008,6 +1098,26 @@ static int mxs_lradc_probe(struct platform_device
>> *pdev) if (ret)
>> goto err_trig;
>>
>> + /* Populate available ADC input ranges */
>> + for (i = 0; i < LRADC_MAX_TOTAL_CHANS; i++) {
>> + for (s = 0; s < ARRAY_SIZE(lradc->scale_avail[i]); s++) {
>> + /*
>> + * [s=0] = optional divider by two disabled (default)
>> + * [s=1] = optional divider by two enabled
>> + *
>> + * The scale is calculated by doing:
>> + * Vref >> (realbits - s)
>> + * which multiplies by two on the second component
>> + * of the array.
>> + */
>> + scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
>> + (iio->channels[i].scan_type.realbits - s);
>
> Thinking about this, this is basically
>
> vref_mv[CHANNEL] * 100 000 000
> scale_uv = --------------------------------
> 2^(12 - s)
>
> Where s can be either 0 or 1.
>
> Why do you multiply it by 100000000 I don't quite understand, but maybe it's
> fully obvious. It should be documented though.
I copied Michael Hennerich in CC who was the author of this math in
drivers/iio/adc/ad7793.c
drivers/staging/iio/adc/ad7192.c
which I copied. Maybe he can explain. It looks like it is just a question of having a
intermediate large enough number to later do the do_div operation, rather than having
a meaningful figure.
>> + lradc->scale_avail[i][s].nano =
>> + do_div(scale_uv, 100000000) * 10;
>
> Are we not loosing precission here?
Yes, but the do_div is modifying the scale_uv parameter with the integer part so I
guess it might be somewhat justified.
As I said before, I tried to do my own math without success so I just copied what was
in other drivers.
>
>> + lradc->scale_avail[i][s].integer = scale_uv;
>> + }
>> + }
>> +
>> /* Configure the hardware. */
>> mxs_lradc_hw_init(lradc);
>
> Best regards,
> Marek Vasut
>
Best regards,
--
Hector Palacios
On 07/22/2013 04:04 PM, Hector Palacios wrote:
[...]
>
> +static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
> + struct device_attribute *attr,
> + char *buf,
> + int ch)
> +{
> + struct iio_dev *iio = dev_to_iio_dev(dev);
> + struct mxs_lradc *lradc = iio_priv(iio);
> + int i, len = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
> + len += sprintf(buf + len, "%d.%09u ",
> + lradc->scale_avail[ch][i].integer,
> + lradc->scale_avail[ch][i].nano);
> +
> + len += sprintf(buf + len, "\n");
> +
> + return len;
> +}
> +
> +static ssize_t mxs_lradc_show_scale_available(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
> +
> + return mxs_lradc_show_scale_available_ch(dev, attr, buf,
> + iio_attr->address);
> +}
> +
> +#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO, \
> + mxs_lradc_show_scale_available, NULL, ch)
> +
> +SHOW_SCALE_AVAILABLE_ATTR(0);
> +SHOW_SCALE_AVAILABLE_ATTR(1);
> +SHOW_SCALE_AVAILABLE_ATTR(2);
> +SHOW_SCALE_AVAILABLE_ATTR(3);
> +SHOW_SCALE_AVAILABLE_ATTR(4);
> +SHOW_SCALE_AVAILABLE_ATTR(5);
> +SHOW_SCALE_AVAILABLE_ATTR(6);
> +SHOW_SCALE_AVAILABLE_ATTR(7);
> +SHOW_SCALE_AVAILABLE_ATTR(8);
> +SHOW_SCALE_AVAILABLE_ATTR(9);
> +SHOW_SCALE_AVAILABLE_ATTR(10);
> +SHOW_SCALE_AVAILABLE_ATTR(11);
> +SHOW_SCALE_AVAILABLE_ATTR(12);
> +SHOW_SCALE_AVAILABLE_ATTR(13);
> +SHOW_SCALE_AVAILABLE_ATTR(14);
> +SHOW_SCALE_AVAILABLE_ATTR(15);
> +
> +static struct attribute *mxs_lradc_attributes[] = {
> + &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
> + &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
> + NULL
> +};
This should really be using the iio_chan_spec_ext_info infrastructure. Bonus
points for factoring out the common code used to calculate and display the
scales.
- Lars
Dear Lars,
On 07/23/2013 10:46 AM, Lars-Peter Clausen wrote:
> On 07/22/2013 04:04 PM, Hector Palacios wrote:
> [...]
>>
>> +static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf,
>> + int ch)
>> +{
>> + struct iio_dev *iio = dev_to_iio_dev(dev);
>> + struct mxs_lradc *lradc = iio_priv(iio);
>> + int i, len = 0;
>> +
>> + for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
>> + len += sprintf(buf + len, "%d.%09u ",
>> + lradc->scale_avail[ch][i].integer,
>> + lradc->scale_avail[ch][i].nano);
>> +
>> + len += sprintf(buf + len, "\n");
>> +
>> + return len;
>> +}
>> +
>> +static ssize_t mxs_lradc_show_scale_available(struct device *dev,
>> + struct device_attribute *attr,
>> + char *buf)
>> +{
>> + struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>> +
>> + return mxs_lradc_show_scale_available_ch(dev, attr, buf,
>> + iio_attr->address);
>> +}
>> +
>> +#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
>> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO, \
>> + mxs_lradc_show_scale_available, NULL, ch)
>> +
>> +SHOW_SCALE_AVAILABLE_ATTR(0);
>> +SHOW_SCALE_AVAILABLE_ATTR(1);
>> +SHOW_SCALE_AVAILABLE_ATTR(2);
>> +SHOW_SCALE_AVAILABLE_ATTR(3);
>> +SHOW_SCALE_AVAILABLE_ATTR(4);
>> +SHOW_SCALE_AVAILABLE_ATTR(5);
>> +SHOW_SCALE_AVAILABLE_ATTR(6);
>> +SHOW_SCALE_AVAILABLE_ATTR(7);
>> +SHOW_SCALE_AVAILABLE_ATTR(8);
>> +SHOW_SCALE_AVAILABLE_ATTR(9);
>> +SHOW_SCALE_AVAILABLE_ATTR(10);
>> +SHOW_SCALE_AVAILABLE_ATTR(11);
>> +SHOW_SCALE_AVAILABLE_ATTR(12);
>> +SHOW_SCALE_AVAILABLE_ATTR(13);
>> +SHOW_SCALE_AVAILABLE_ATTR(14);
>> +SHOW_SCALE_AVAILABLE_ATTR(15);
>> +
>> +static struct attribute *mxs_lradc_attributes[] = {
>> + &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
>> + &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
>> + NULL
>> +};
>
> This should really be using the iio_chan_spec_ext_info infrastructure. Bonus
> points for factoring out the common code used to calculate and display the
> scales.
I perfectly understand. Sadly, I don't currently have the time and expertise to try to
work this out the proper way. It already took much longer than expected to have this
driver toggle a divider flag.
I'd appreciate if anyone wishes to complete this job.
@Alexander, please feel free to submit your other temp patch without waiting for this one.
Best regards,
--
Hector Palacios
On 23/07/2013 00:06, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
>
>> On 07/22/2013 04:04 PM, Hector Palacios wrote:
>>> Some LRADC channels have fixed pre-dividers so they can measure
>>> different voltages at full scale. The reference voltage allows to
>>> expose a scaling attribute through the IIO sysfs so that a user can
>>> compute the real voltage out of a measured sample value.
>> I've said before that I'm not convinced that this is the right way to
>> implement this. And considering what Thomas said here
>> http://www.mail-archive.com/[email protected]/msg36691.ht
>> ml I guess I'm not alone with that opinion.
>>
>> - Lars
> He's talking about different versions of the IP block, this is the same IP
> block, just connected to different inputs.
I don't have any strong opinion on that and I'm certainly not an expert
but thinking about that, I feel this is different from the discussion
Thomas had. Those are reference voltages and describe how the IP block
is connected whereas what Thomas was describing is supporting multiple
versions of an IP block by describing heaps of registers in the DT. It
may look ugly here because you have a lot of channels but I find that
quite acceptable.
Regards,
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 23/07/2013 15:25, Hector Palacios wrote:
> Dear Lars,
>
> On 07/23/2013 10:46 AM, Lars-Peter Clausen wrote:
>> On 07/22/2013 04:04 PM, Hector Palacios wrote:
>> [...]
>>>
>>> +static ssize_t mxs_lradc_show_scale_available_ch(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf,
>>> + int ch)
>>> +{
>>> + struct iio_dev *iio = dev_to_iio_dev(dev);
>>> + struct mxs_lradc *lradc = iio_priv(iio);
>>> + int i, len = 0;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
>>> + len += sprintf(buf + len, "%d.%09u ",
>>> + lradc->scale_avail[ch][i].integer,
>>> + lradc->scale_avail[ch][i].nano);
>>> +
>>> + len += sprintf(buf + len, "\n");
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static ssize_t mxs_lradc_show_scale_available(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>>> +
>>> + return mxs_lradc_show_scale_available_ch(dev, attr, buf,
>>> + iio_attr->address);
>>> +}
>>> +
>>> +#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
>>> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO, \
>>> + mxs_lradc_show_scale_available, NULL, ch)
>>> +
>>> +SHOW_SCALE_AVAILABLE_ATTR(0);
>>> +SHOW_SCALE_AVAILABLE_ATTR(1);
>>> +SHOW_SCALE_AVAILABLE_ATTR(2);
>>> +SHOW_SCALE_AVAILABLE_ATTR(3);
>>> +SHOW_SCALE_AVAILABLE_ATTR(4);
>>> +SHOW_SCALE_AVAILABLE_ATTR(5);
>>> +SHOW_SCALE_AVAILABLE_ATTR(6);
>>> +SHOW_SCALE_AVAILABLE_ATTR(7);
>>> +SHOW_SCALE_AVAILABLE_ATTR(8);
>>> +SHOW_SCALE_AVAILABLE_ATTR(9);
>>> +SHOW_SCALE_AVAILABLE_ATTR(10);
>>> +SHOW_SCALE_AVAILABLE_ATTR(11);
>>> +SHOW_SCALE_AVAILABLE_ATTR(12);
>>> +SHOW_SCALE_AVAILABLE_ATTR(13);
>>> +SHOW_SCALE_AVAILABLE_ATTR(14);
>>> +SHOW_SCALE_AVAILABLE_ATTR(15);
>>> +
>>> +static struct attribute *mxs_lradc_attributes[] = {
>>> + &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
>>> + &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
>>> + NULL
>>> +};
>>
>> This should really be using the iio_chan_spec_ext_info
>> infrastructure. Bonus
>> points for factoring out the common code used to calculate and
>> display the
>> scales.
>
> I perfectly understand. Sadly, I don't currently have the time and
> expertise to try to work this out the proper way. It already took much
> longer than expected to have this driver toggle a divider flag.
>
> I'd appreciate if anyone wishes to complete this job.
> @Alexander, please feel free to submit your other temp patch without
> waiting for this one.
>
Maybe, we can get the patch set as is and do further clean up later.
Anyway, that driver is still in staging, right ?
As said, I'm willing to propose something for the scale calculation.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Alexandre Belloni <[email protected]> wrote:
>On 23/07/2013 15:25, Hector Palacios wrote:
>> Dear Lars,
>>
>> On 07/23/2013 10:46 AM, Lars-Peter Clausen wrote:
>>> On 07/22/2013 04:04 PM, Hector Palacios wrote:
>>> [...]
>>>>
>>>> +static ssize_t mxs_lradc_show_scale_available_ch(struct device
>*dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf,
>>>> + int ch)
>>>> +{
>>>> + struct iio_dev *iio = dev_to_iio_dev(dev);
>>>> + struct mxs_lradc *lradc = iio_priv(iio);
>>>> + int i, len = 0;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
>>>> + len += sprintf(buf + len, "%d.%09u ",
>>>> + lradc->scale_avail[ch][i].integer,
>>>> + lradc->scale_avail[ch][i].nano);
>>>> +
>>>> + len += sprintf(buf + len, "\n");
>>>> +
>>>> + return len;
>>>> +}
>>>> +
>>>> +static ssize_t mxs_lradc_show_scale_available(struct device *dev,
>>>> + struct device_attribute *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>>>> +
>>>> + return mxs_lradc_show_scale_available_ch(dev, attr, buf,
>>>> + iio_attr->address);
>>>> +}
>>>> +
>>>> +#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
>>>> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO,
> \
>>>> + mxs_lradc_show_scale_available, NULL, ch)
>>>> +
>>>> +SHOW_SCALE_AVAILABLE_ATTR(0);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(1);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(2);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(3);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(4);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(5);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(6);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(7);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(8);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(9);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(10);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(11);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(12);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(13);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(14);
>>>> +SHOW_SCALE_AVAILABLE_ATTR(15);
>>>> +
>>>> +static struct attribute *mxs_lradc_attributes[] = {
>>>> + &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
>>>> + &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
>>>> + NULL
>>>> +};
>>>
>>> This should really be using the iio_chan_spec_ext_info
>>> infrastructure. Bonus
>>> points for factoring out the common code used to calculate and
>>> display the
>>> scales.
>>
>> I perfectly understand. Sadly, I don't currently have the time and
>> expertise to try to work this out the proper way. It already took
>much
>> longer than expected to have this driver toggle a divider flag.
>>
>> I'd appreciate if anyone wishes to complete this job.
>> @Alexander, please feel free to submit your other temp patch without
>> waiting for this one.
>>
>
>Maybe, we can get the patch set as is and do further clean up later.
>Anyway, that driver is still in staging, right ?
>
>As said, I'm willing to propose something for the scale calculation.
My thoughts exactly. Long term plans include generic handling of available attributes for the whole of IIO.
It will take me a little while to catch up my backlog though!
Jonathan
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Hi Jonathan, Lars
I would really like to see that included in 3.12. While I agree there is
room for improvement, I don't think there are any comments that need an
immediate action before inclusion apart from the decision that has to be
taken for the device tree.
Can you confirm whether you'll take this series as is ?
Regards,
On 26/07/2013 18:13, Jonathan Cameron wrote:
>
> Alexandre Belloni <[email protected]> wrote:
>> On 23/07/2013 15:25, Hector Palacios wrote:
>>> Dear Lars,
>>>
>>> On 07/23/2013 10:46 AM, Lars-Peter Clausen wrote:
>>>> On 07/22/2013 04:04 PM, Hector Palacios wrote:
>>>> [...]
>>>>> +static ssize_t mxs_lradc_show_scale_available_ch(struct device
>> *dev,
>>>>> + struct device_attribute *attr,
>>>>> + char *buf,
>>>>> + int ch)
>>>>> +{
>>>>> + struct iio_dev *iio = dev_to_iio_dev(dev);
>>>>> + struct mxs_lradc *lradc = iio_priv(iio);
>>>>> + int i, len = 0;
>>>>> +
>>>>> + for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
>>>>> + len += sprintf(buf + len, "%d.%09u ",
>>>>> + lradc->scale_avail[ch][i].integer,
>>>>> + lradc->scale_avail[ch][i].nano);
>>>>> +
>>>>> + len += sprintf(buf + len, "\n");
>>>>> +
>>>>> + return len;
>>>>> +}
>>>>> +
>>>>> +static ssize_t mxs_lradc_show_scale_available(struct device *dev,
>>>>> + struct device_attribute *attr,
>>>>> + char *buf)
>>>>> +{
>>>>> + struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>>>>> +
>>>>> + return mxs_lradc_show_scale_available_ch(dev, attr, buf,
>>>>> + iio_attr->address);
>>>>> +}
>>>>> +
>>>>> +#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
>>>>> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO,
>> \
>>>>> + mxs_lradc_show_scale_available, NULL, ch)
>>>>> +
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(0);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(1);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(2);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(3);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(4);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(5);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(6);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(7);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(8);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(9);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(10);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(11);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(12);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(13);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(14);
>>>>> +SHOW_SCALE_AVAILABLE_ATTR(15);
>>>>> +
>>>>> +static struct attribute *mxs_lradc_attributes[] = {
>>>>> + &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
>>>>> + &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
>>>>> + NULL
>>>>> +};
>>>> This should really be using the iio_chan_spec_ext_info
>>>> infrastructure. Bonus
>>>> points for factoring out the common code used to calculate and
>>>> display the
>>>> scales.
>>> I perfectly understand. Sadly, I don't currently have the time and
>>> expertise to try to work this out the proper way. It already took
>> much
>>> longer than expected to have this driver toggle a divider flag.
>>>
>>> I'd appreciate if anyone wishes to complete this job.
>>> @Alexander, please feel free to submit your other temp patch without
>>> waiting for this one.
>>>
>> Maybe, we can get the patch set as is and do further clean up later.
>> Anyway, that driver is still in staging, right ?
>>
>> As said, I'm willing to propose something for the scale calculation.
> My thoughts exactly. Long term plans include generic handling of available attributes for the whole of IIO.
>
> It will take me a little while to catch up my backlog though!
>
> Jonathan
>
>
>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 07/22/13 15:04, Hector Palacios wrote:
> Some LRADC channels have fixed pre-dividers so they can measure
> different voltages at full scale. The reference voltage allows to
> expose a scaling attribute through the IIO sysfs so that a user can
> compute the real voltage out of a measured sample value.
>
> Signed-off-by: Hector Palacios <[email protected]>
> Acked-by: Marek Vasut <[email protected]>
> Acked-by: Alexandre Belloni <[email protected]>
CC'd the device tree maintainers to see if I can get a comment on this one.
(done from this initial posting of the patch so that the code is still present).
I've avoided the patch for a while because opinion was divided.
Lars-Peter Clausen commented:
I've said before that I'm not convinced that this is the right way to implement this. And considering what Thomas said
here http://www.mail-archive.com/[email protected]/msg36691.html I guess I'm not alone with that opinion.
Marek replied:
He's talking about different versions of the IP block, this is the same IP
block, just connected to different inputs.
Alexander added:
I don't have any strong opinion on that and I'm certainly not an expert
but thinking about that, I feel this is different from the discussion
Thomas had. Those are reference voltages and describe how the IP block
is connected whereas what Thomas was describing is supporting multiple
versions of an IP block by describing heaps of registers in the DT. It
may look ugly here because you have a lot of channels but I find that
quite acceptable.
Personally I'm falling slightly on the side of this being a bad idea as the
fact they are specified in the processor device tree files and clearly lines
up with the compatible strings suggests this is ought to be in the driver
not the device tree.
Anyhow, looking for more opinions / acks as appropriate.
Incidentally I'm very glad that people have come forward to help with
this stuff!
Jonathan
> ---
> Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt | 9 ++++++++-
> arch/arm/boot/dts/imx23.dtsi | 4 ++++
> arch/arm/boot/dts/imx28.dtsi | 4 ++++
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> index 4688205..6ec485c 100644
> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> @@ -1,9 +1,12 @@
> * Freescale i.MX28 LRADC device driver
>
> Required properties:
> -- compatible: Should be "fsl,imx28-lradc"
> +- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
> - reg: Address and length of the register set for the device
> - interrupts: Should contain the LRADC interrupts
> +- fsl,vref: Reference voltage (in mV) for each LRADC channel. This is the
> + maximum voltage that can be measured at full scale in each channel
> + considering fixed pre-dividers.
>
> Optional properties:
> - fsl,lradc-touchscreen-wires: Number of wires used to connect the touchscreen
> @@ -18,4 +21,8 @@ Examples:
> reg = <0x80050000 0x2000>;
> interrupts = <10 14 15 16 17 18 19
> 20 21 22 23 24 25>;
> + fsl,vref = <1850 1850 1850 1850
> + 1850 1850 1850 7400
> + 1850 1850 3700 1850
> + 3700 1850 1850 7400>
> };
> diff --git a/arch/arm/boot/dts/imx23.dtsi b/arch/arm/boot/dts/imx23.dtsi
> index 587ceef..e212902 100644
> --- a/arch/arm/boot/dts/imx23.dtsi
> +++ b/arch/arm/boot/dts/imx23.dtsi
> @@ -430,6 +430,10 @@
> compatible = "fsl,imx23-lradc";
> reg = <0x80050000 0x2000>;
> interrupts = <36 37 38 39 40 41 42 43 44>;
> + fsl,vref = <1850 1850 1850 1850
> + 1850 1850 3700 7400
> + 1850 1850 1850 1850
> + 1850 1850 1850 7400>;
> status = "disabled";
> };
>
> diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
> index 6a8acb0..c1b3724 100644
> --- a/arch/arm/boot/dts/imx28.dtsi
> +++ b/arch/arm/boot/dts/imx28.dtsi
> @@ -865,6 +865,10 @@
> reg = <0x80050000 0x2000>;
> interrupts = <10 14 15 16 17 18 19
> 20 21 22 23 24 25>;
> + fsl,vref = <1850 1850 1850 1850
> + 1850 1850 1850 7400
> + 1850 1850 3700 1850
> + 3700 1850 1850 7400>;
> status = "disabled";
> };
>
>
On 07/22/13 15:03, Hector Palacios wrote:
> The LRADC virtual channels have an 18 bit field to store the sum of up
> to 2^5 accumulated samples. The read_raw function however only operates
> over a single sample (12 bit resolution).
> In order to use this field for scaling operations, we need it to be the
> exact resolution value of the LRADC.
> Besides, the driver was using an 18 bit mask (LRADC_CH_VALUE_MASK) to
> report touch coordinates to userland. A 12 bit mask should be used instead
> or else the touch libraries will expect a coordinates range between 0
> and 0x3ffff (18 bits), instead of between 0 and 0xfff (12 bits).
>
> Signed-off-by: Hector Palacios <[email protected]>
> Acked-by: Marek Vasut <[email protected]>
> Acked-by: Alexandre Belloni <[email protected]>
Applied to the togreg branch of iio.git
Thanks,
Jonathan
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 9f52a28..56667da 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -225,6 +225,9 @@ struct mxs_lradc {
> #define LRADC_CTRL4_LRADCSELECT_MASK(n) (0xf << ((n) * 4))
> #define LRADC_CTRL4_LRADCSELECT_OFFSET(n) ((n) * 4)
>
> +#define LRADC_RESOLUTION 12
> +#define LRADC_SINGLE_SAMPLE_MASK ((1 << LRADC_RESOLUTION) - 1)
> +
> /*
> * Raw I/O operations
> */
> @@ -540,9 +543,10 @@ static int mxs_lradc_ts_register(struct mxs_lradc *lradc)
> __set_bit(EV_ABS, input->evbit);
> __set_bit(EV_KEY, input->evbit);
> __set_bit(BTN_TOUCH, input->keybit);
> - input_set_abs_params(input, ABS_X, 0, LRADC_CH_VALUE_MASK, 0, 0);
> - input_set_abs_params(input, ABS_Y, 0, LRADC_CH_VALUE_MASK, 0, 0);
> - input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_CH_VALUE_MASK, 0, 0);
> + input_set_abs_params(input, ABS_X, 0, LRADC_SINGLE_SAMPLE_MASK, 0, 0);
> + input_set_abs_params(input, ABS_Y, 0, LRADC_SINGLE_SAMPLE_MASK, 0, 0);
> + input_set_abs_params(input, ABS_PRESSURE, 0, LRADC_SINGLE_SAMPLE_MASK,
> + 0, 0);
>
> lradc->ts_input = input;
> input_set_drvdata(input, lradc);
> @@ -817,7 +821,7 @@ static const struct iio_buffer_setup_ops mxs_lradc_buffer_ops = {
> .channel = (idx), \
> .scan_type = { \
> .sign = 'u', \
> - .realbits = 18, \
> + .realbits = LRADC_RESOLUTION, \
> .storagebits = 32, \
> }, \
> }
>
On 08/07/13 08:50, Alexandre Belloni wrote:
> Hi Jonathan, Lars
>
> I would really like to see that included in 3.12. While I agree there is
> room for improvement, I don't think there are any comments that need an
> immediate action before inclusion apart from the decision that has to be
> taken for the device tree.
>
> Can you confirm whether you'll take this series as is ?
I've just raised the device tree issue again. I'm happy with everything
else in the series, but want some more expert input on that small element.
Sorry this has taken so long and hopefully we'll get it cleared up over the
next couple of days.
Jonathan
>
> Regards,
>
> On 26/07/2013 18:13, Jonathan Cameron wrote:
>>
>> Alexandre Belloni <[email protected]> wrote:
>>> On 23/07/2013 15:25, Hector Palacios wrote:
>>>> Dear Lars,
>>>>
>>>> On 07/23/2013 10:46 AM, Lars-Peter Clausen wrote:
>>>>> On 07/22/2013 04:04 PM, Hector Palacios wrote:
>>>>> [...]
>>>>>> +static ssize_t mxs_lradc_show_scale_available_ch(struct device
>>> *dev,
>>>>>> + struct device_attribute *attr,
>>>>>> + char *buf,
>>>>>> + int ch)
>>>>>> +{
>>>>>> + struct iio_dev *iio = dev_to_iio_dev(dev);
>>>>>> + struct mxs_lradc *lradc = iio_priv(iio);
>>>>>> + int i, len = 0;
>>>>>> +
>>>>>> + for (i = 0; i < ARRAY_SIZE(lradc->scale_avail[ch]); i++)
>>>>>> + len += sprintf(buf + len, "%d.%09u ",
>>>>>> + lradc->scale_avail[ch][i].integer,
>>>>>> + lradc->scale_avail[ch][i].nano);
>>>>>> +
>>>>>> + len += sprintf(buf + len, "\n");
>>>>>> +
>>>>>> + return len;
>>>>>> +}
>>>>>> +
>>>>>> +static ssize_t mxs_lradc_show_scale_available(struct device *dev,
>>>>>> + struct device_attribute *attr,
>>>>>> + char *buf)
>>>>>> +{
>>>>>> + struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
>>>>>> +
>>>>>> + return mxs_lradc_show_scale_available_ch(dev, attr, buf,
>>>>>> + iio_attr->address);
>>>>>> +}
>>>>>> +
>>>>>> +#define SHOW_SCALE_AVAILABLE_ATTR(ch) \
>>>>>> +static IIO_DEVICE_ATTR(in_voltage##ch##_scale_available, S_IRUGO,
>>> \
>>>>>> + mxs_lradc_show_scale_available, NULL, ch)
>>>>>> +
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(0);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(1);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(2);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(3);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(4);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(5);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(6);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(7);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(8);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(9);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(10);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(11);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(12);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(13);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(14);
>>>>>> +SHOW_SCALE_AVAILABLE_ATTR(15);
>>>>>> +
>>>>>> +static struct attribute *mxs_lradc_attributes[] = {
>>>>>> + &iio_dev_attr_in_voltage0_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage1_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage2_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage3_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage4_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage5_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage6_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage7_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage8_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage9_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage10_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage11_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage12_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage13_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage14_scale_available.dev_attr.attr,
>>>>>> + &iio_dev_attr_in_voltage15_scale_available.dev_attr.attr,
>>>>>> + NULL
>>>>>> +};
>>>>> This should really be using the iio_chan_spec_ext_info
>>>>> infrastructure. Bonus
>>>>> points for factoring out the common code used to calculate and
>>>>> display the
>>>>> scales.
>>>> I perfectly understand. Sadly, I don't currently have the time and
>>>> expertise to try to work this out the proper way. It already took
>>> much
>>>> longer than expected to have this driver toggle a divider flag.
>>>>
>>>> I'd appreciate if anyone wishes to complete this job.
>>>> @Alexander, please feel free to submit your other temp patch without
>>>> waiting for this one.
>>>>
>>> Maybe, we can get the patch set as is and do further clean up later.
>>> Anyway, that driver is still in staging, right ?
>>>
>>> As said, I'm willing to propose something for the scale calculation.
>> My thoughts exactly. Long term plans include generic handling of available attributes for the whole of IIO.
>>
>> It will take me a little while to catch up my backlog though!
>>
>> Jonathan
>>
>>
>>
>
>
On Tue, 2013-08-13 at 22:23 +0100, Jonathan Cameron wrote:
> On 07/22/13 15:04, Hector Palacios wrote:
> > Some LRADC channels have fixed pre-dividers so they can measure
> > different voltages at full scale. The reference voltage allows to
> > expose a scaling attribute through the IIO sysfs so that a user can
> > compute the real voltage out of a measured sample value.
> >
> > diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> > index 4688205..6ec485c 100644
> > --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> > +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
> > @@ -1,9 +1,12 @@
> > * Freescale i.MX28 LRADC device driver
> >
> > Required properties:
> > -- compatible: Should be "fsl,imx28-lradc"
> > +- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
> > - reg: Address and length of the register set for the device
> > - interrupts: Should contain the LRADC interrupts
> > +- fsl,vref: Reference voltage (in mV) for each LRADC channel. This is the
> > + maximum voltage that can be measured at full scale in each channel
> > + considering fixed pre-dividers.
So, let me try to rephrase what I read above.
There's an ADC with X channels. And there's a reference voltage source
(one?). Now, each of the ADC channels have a (different?) voltage
divider, taking the voltage from the reference source and feeding it to
the ADC comparator. How much am I wrong?
If I'm not wrong at all, I'd say that the reference source could be
described as a standard fixed regulator
(Documentation/devicetree/bindings/regulator/fixed-regulator.txt) and
the ADC node should have some king of "reference-supply" phandle to the
regulator node. Now, if the dividers factors are *really* fixed, the
driver could know about them and calculate the effective reference
voltage on its own, couldn't it?
Let me repeat the "DT standard disclaimer": the tree, in general, should
describe the way components are *wired up*, not much more.
Pawel
Hi Pawel,
On 14/08/2013 16:44, Pawel Moll wrote:
> On Tue, 2013-08-13 at 22:23 +0100, Jonathan Cameron wrote:
>> On 07/22/13 15:04, Hector Palacios wrote:
>>> Some LRADC channels have fixed pre-dividers so they can measure
>>> different voltages at full scale. The reference voltage allows to
>>> expose a scaling attribute through the IIO sysfs so that a user can
>>> compute the real voltage out of a measured sample value.
>>>
>>> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> index 4688205..6ec485c 100644
>>> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>> @@ -1,9 +1,12 @@
>>> * Freescale i.MX28 LRADC device driver
>>>
>>> Required properties:
>>> -- compatible: Should be "fsl,imx28-lradc"
>>> +- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
>>> - reg: Address and length of the register set for the device
>>> - interrupts: Should contain the LRADC interrupts
>>> +- fsl,vref: Reference voltage (in mV) for each LRADC channel. This is the
>>> + maximum voltage that can be measured at full scale in each channel
>>> + considering fixed pre-dividers.
>
> So, let me try to rephrase what I read above.
>
> There's an ADC with X channels. And there's a reference voltage source
> (one?). Now, each of the ADC channels have a (different?) voltage
> divider, taking the voltage from the reference source and feeding it to
> the ADC comparator. How much am I wrong?
>
You are not so wrong. There is indeed actually only one reference
voltage (and that is 1.85V). But, before feeding the voltage to the ADC
channels, you sometimes have a divider. Then, after the channel muxing,
you can add a by 2 divider.
Mandatory ascii art:
+-----+
| |
+-ch1--->| |
| |
| |
| | +-----+
+-ch2--->| | | |
| MUX |++-->| ADC +----------->
ch3 | | | | |
+----+ | | | +-----+
| | | | | |
+-> :4 +->| | | +---+--+
| | | | | | |
+----+ | | +->| :2 |
+-----+ | |
+------+
> If I'm not wrong at all, I'd say that the reference source could be
> described as a standard fixed regulator
> (Documentation/devicetree/bindings/regulator/fixed-regulator.txt) and
> the ADC node should have some king of "reference-supply" phandle to the
> regulator node. Now, if the dividers factors are *really* fixed, the
> driver could know about them and calculate the effective reference
> voltage on its own, couldn't it?
>
> Let me repeat the "DT standard disclaimer": the tree, in general, should
> describe the way components are *wired up*, not much more.
>
So, from my point of view, the divider that is before the mux (the by 4
divider on channel 3 on my drawing) is not part of the the ADC, it is
not fixed by that IP. And indeed, that changed between the i.mx23 and
i.mx28 while the IP is the same.
So, the two solutions you suggest are:
1/ using a fixed-regulator phandle per channel
2/ hard-coding the dividers in the driver using the compatible string to
know which divider is on which channel.
I feel that solution 2 is less future proof but at the same time, I
don't believe we will see that IP in another chip in the future.
Are my explanations clear enough to take a decision ?
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Sorry accidental HTML message hence resend
Alexandre Belloni <[email protected]> wrote:
>Hi Pawel,
>
>On 14/08/2013 16:44, Pawel Moll wrote:
>> On Tue, 2013-08-13 at 22:23 +0100, Jonathan Cameron wrote:
>>> On 07/22/13 15:04, Hector Palacios wrote:
>>>> Some LRADC channels have fixed pre-dividers so they can measure
>>>> different voltages at full scale. The reference voltage allows to
>>>> expose a scaling attribute through the IIO sysfs so that a user can
>>>> compute the real voltage out of a measured sample value.
>>>>
>>>> diff --git
>a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> index 4688205..6ec485c 100644
>>>> ---
>a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> +++
>b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> @@ -1,9 +1,12 @@
>>>> * Freescale i.MX28 LRADC device driver
>>>>
>>>> Required properties:
>>>> -- compatible: Should be "fsl,imx28-lradc"
>>>> +- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
>>>> - reg: Address and length of the register set for the device
>>>> - interrupts: Should contain the LRADC interrupts
>>>> +- fsl,vref: Reference voltage (in mV) for each LRADC channel. This
>is the
>>>> + maximum voltage that can be measured at full scale in each
>channel
>>>> + considering fixed pre-dividers.
>>
>> So, let me try to rephrase what I read above.
>>
>> There's an ADC with X channels. And there's a reference voltage
>source
>> (one?). Now, each of the ADC channels have a (different?) voltage
>> divider, taking the voltage from the reference source and feeding it
>to
>> the ADC comparator. How much am I wrong?
>>
>
>You are not so wrong. There is indeed actually only one reference
>voltage (and that is 1.85V). But, before feeding the voltage to the ADC
>channels, you sometimes have a divider. Then, after the channel muxing,
>you can add a by 2 divider.
>
>Mandatory ascii art:
>
> +-----+
> | |
> +-ch1--->| |
> | |
> | |
> | | +-----+
> +-ch2--->| | | |
> | MUX |++-->| ADC +----------->
> ch3 | | | | |
> +----+ | | | +-----+
> | | | | | |
> +-> :4 +->| | | +---+--+
> | | | | | | |
> +----+ | | +->| :2 |
> +-----+ | |
> +------+
>
>
>> If I'm not wrong at all, I'd say that the reference source could be
>> described as a standard fixed regulator
>> (Documentation/devicetree/bindings/regulator/fixed-regulator.txt) and
>> the ADC node should have some king of "reference-supply" phandle to
>the
>> regulator node. Now, if the dividers factors are *really* fixed, the
>> driver could know about them and calculate the effective reference
>> voltage on its own, couldn't it?
>>
>> Let me repeat the "DT standard disclaimer": the tree, in general,
>should
>> describe the way components are *wired up*, not much more.
>>
>
>So, from my point of view, the divider that is before the mux (the by 4
>divider on channel 3 on my drawing) is not part of the the ADC, it is
>not fixed by that IP. And indeed, that changed between the i.mx23 and
>i.mx28 while the IP is the same.
>
>So, the two solutions you suggest are:
>1/ using a fixed-regulator phandle per channel
>2/ hard-coding the dividers in the driver using the compatible string
>to
>know which divider is on which channel.
>
>I feel that solution 2 is less future proof but at the same time, I
>don't believe we will see that IP in another chip in the future.
>
>Are my explanations clear enough to take a decision ?
>
>--
>Alexandre Belloni, Free Electrons
>Embedded Linux, Kernel and Android engineering
>http://free-electrons.com
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to [email protected]
>More majordomo info at http://vger.kernel.org/majordomo-info.html
I would favour option 2 though some of the discussions going on at the moment about
bindings might result in a generic description of this and any other bits of
analog front end.
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Dear Alexandre,
On 08/22/2013 12:13 AM, Alexandre Belloni wrote:
> Hi Pawel,
>
> On 14/08/2013 16:44, Pawel Moll wrote:
>> On Tue, 2013-08-13 at 22:23 +0100, Jonathan Cameron wrote:
>>> On 07/22/13 15:04, Hector Palacios wrote:
>>>> Some LRADC channels have fixed pre-dividers so they can measure
>>>> different voltages at full scale. The reference voltage allows to
>>>> expose a scaling attribute through the IIO sysfs so that a user can
>>>> compute the real voltage out of a measured sample value.
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> index 4688205..6ec485c 100644
>>>> --- a/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> +++ b/Documentation/devicetree/bindings/staging/iio/adc/mxs-lradc.txt
>>>> @@ -1,9 +1,12 @@
>>>> * Freescale i.MX28 LRADC device driver
>>>>
>>>> Required properties:
>>>> -- compatible: Should be "fsl,imx28-lradc"
>>>> +- compatible: "fsl,imx28-lradc", "fsl,imx23-lradc"
>>>> - reg: Address and length of the register set for the device
>>>> - interrupts: Should contain the LRADC interrupts
>>>> +- fsl,vref: Reference voltage (in mV) for each LRADC channel. This is the
>>>> + maximum voltage that can be measured at full scale in each channel
>>>> + considering fixed pre-dividers.
>>
>> So, let me try to rephrase what I read above.
>>
>> There's an ADC with X channels. And there's a reference voltage source
>> (one?). Now, each of the ADC channels have a (different?) voltage
>> divider, taking the voltage from the reference source and feeding it to
>> the ADC comparator. How much am I wrong?
>>
>
> You are not so wrong. There is indeed actually only one reference
> voltage (and that is 1.85V). But, before feeding the voltage to the ADC
> channels, you sometimes have a divider. Then, after the channel muxing,
> you can add a by 2 divider.
>
> Mandatory ascii art:
>
> +-----+
> | |
> +-ch1--->| |
> | |
> | |
> | | +-----+
> +-ch2--->| | | |
> | MUX |++-->| ADC +----------->
> ch3 | | | | |
> +----+ | | | +-----+
> | | | | | |
> +-> :4 +->| | | +---+--+
> | | | | | | |
> +----+ | | +->| :2 |
> +-----+ | |
> +------+
>
>
>> If I'm not wrong at all, I'd say that the reference source could be
>> described as a standard fixed regulator
>> (Documentation/devicetree/bindings/regulator/fixed-regulator.txt) and
>> the ADC node should have some king of "reference-supply" phandle to the
>> regulator node. Now, if the dividers factors are *really* fixed, the
>> driver could know about them and calculate the effective reference
>> voltage on its own, couldn't it?
>>
>> Let me repeat the "DT standard disclaimer": the tree, in general, should
>> describe the way components are *wired up*, not much more.
>>
>
> So, from my point of view, the divider that is before the mux (the by 4
> divider on channel 3 on my drawing) is not part of the the ADC, it is
> not fixed by that IP. And indeed, that changed between the i.mx23 and
> i.mx28 while the IP is the same.
The dividers only make sense and affect the ADC, so whether they should be considered
part of the ADC IP or not is a philosophical question.
In my opinion, the different dividers between the i.mx23 and i.mx28 channels are the
kind of hardware differences that fit nicely in the DeviceTree, describing the hardware.
> So, the two solutions you suggest are:
> 1/ using a fixed-regulator phandle per channel
Since the dividers only affect and have meaning on the ADC channels, creating a
regulator for each channel that has a different divider looks to me like an overworked
solution. These are not real voltage sources. They are just indicators of the maximum
reference voltage that an ADC channel can measure.
> 2/ hard-coding the dividers in the driver using the compatible string to
> know which divider is on which channel.
>
> I feel that solution 2 is less future proof but at the same time, I
> don't believe we will see that IP in another chip in the future.
This was what I originally submitted but it then looked like it would better fit in
the DeviceTree. The spear-adc seemed to use a similar approach:
http://permalink.gmane.org/gmane.linux.kernel.iio/7994
Best regards,
--
Hector Palacios
On Wed, 2013-08-21 at 23:13 +0100, Alexandre Belloni wrote:
> You are not so wrong. There is indeed actually only one reference
> voltage (and that is 1.85V). But, before feeding the voltage to the ADC
> channels, you sometimes have a divider. Then, after the channel muxing,
> you can add a by 2 divider.
>
> Mandatory ascii art:
>
> +-----+
> | |
> +-ch1--->| |
> | |
> | |
> | | +-----+
> +-ch2--->| | | |
> | MUX |++-->| ADC +----------->
> ch3 | | | | |
> +----+ | | | +-----+
> | | | | | |
> +-> :4 +->| | | +---+--+
> | | | | | | |
> +----+ | | +->| :2 |
> +-----+ | |
> +------+
>
>
> So, from my point of view, the divider that is before the mux (the by 4
> divider on channel 3 on my drawing) is not part of the the ADC, it is
> not fixed by that IP. And indeed, that changed between the i.mx23 and
> i.mx28 while the IP is the same.
Let me a couple of additional questions, hope you don't mind:
1. Is the channel defined as: input *and* the reference voltage? Or,
does the mux switch both of them at the same time?
2. Is the mux controlled (so the channel selected) by a control register
"integral" to the ADC?
3. Is the reference voltage generated "inside" the SOC? Or does it come
from an external source?
4. How is the "LRADC" IP actually documented? Does the spec clearly say
that it has 8 voltage reference inputs?
> So, the two solutions you suggest are:
> 1/ using a fixed-regulator phandle per channel
> 2/ hard-coding the dividers in the driver using the compatible string to
> know which divider is on which channel.
>
> I feel that solution 2 is less future proof but at the same time, I
> don't believe we will see that IP in another chip in the future.
If we were to follow the spirit of "how is it wired" to the letter, you
should really use 8 supplies, but I appreciate that it can be
troublesome (or maybe not? it's just 2 dtsi files after all ;-). So
maybe, as the compatible values explicitly mention the SOC names, you
just want to hardcode the voltage levels in the driver itself (probably
as data for the match array)? This of course assume that the reference
source is internal. Shortly speaking - I believe that you should have
phandles to regulators or nothing at all there :-) A de-facto-constant
list of SOC-specific numbers seems the worst option.
Thanks!
Pawel
Paweł
On Thu, 2013-08-22 at 09:05 +0100, Hector Palacios wrote:
> This was what I originally submitted but it then looked like it would better fit in
> the DeviceTree. The spear-adc seemed to use a similar approach:
I'm not sure if it's the best example... I much more prefer the AD7303's
way:
Documentation/devicetree/bindings/iio/dac/ad7303.txt
Optional properties:
- REF-supply: Phandle to the external reference voltage supply. This should
only be set if there is an external reference voltage connected to the REF
pin. If the property is not set Vdd/2 is used as the reference voltage.
Paweł
On Thu, 2013-08-22 at 07:17 +0100, Jonathan Cameron wrote:
> I would favour option 2 though some of the discussions going on at the moment about
> bindings might result in a generic description of this and any other bits of
> analog front end.
Any link to this discussion? [email protected] doesn't seem to
be involved (of course I may have simply missed the thread ;-)
Paweł
On 08/22/2013 06:41 PM, Pawel Moll wrote:
>
> On Wed, 2013-08-21 at 23:13 +0100, Alexandre Belloni wrote:
>> You are not so wrong. There is indeed actually only one reference
>> voltage (and that is 1.85V). But, before feeding the voltage to the ADC
>> channels, you sometimes have a divider. Then, after the channel muxing,
>> you can add a by 2 divider.
>>
>> Mandatory ascii art:
>>
>> +-----+
>> | |
>> +-ch1--->| |
>> | |
>> | |
>> | | +-----+
>> +-ch2--->| | | |
>> | MUX |++-->| ADC +----------->
>> ch3 | | | | |
>> +----+ | | | +-----+
>> | | | | | |
>> +-> :4 +->| | | +---+--+
>> | | | | | | |
>> +----+ | | +->| :2 |
>> +-----+ | |
>> +------+
>>
>>
>> So, from my point of view, the divider that is before the mux (the by 4
>> divider on channel 3 on my drawing) is not part of the the ADC, it is
>> not fixed by that IP. And indeed, that changed between the i.mx23 and
>> i.mx28 while the IP is the same.
>
> Let me a couple of additional questions, hope you don't mind:
>
> 1. Is the channel defined as: input *and* the reference voltage? Or,
> does the mux switch both of them at the same time?
>
> 2. Is the mux controlled (so the channel selected) by a control register
> "integral" to the ADC?
>
> 3. Is the reference voltage generated "inside" the SOC? Or does it come
> from an external source?
>
> 4. How is the "LRADC" IP actually documented? Does the spec clearly say
> that it has 8 voltage reference inputs?
There is one internal vref always fixed to the same voltage (I think).
>
>> So, the two solutions you suggest are:
>> 1/ using a fixed-regulator phandle per channel
>> 2/ hard-coding the dividers in the driver using the compatible string to
>> know which divider is on which channel.
>>
>> I feel that solution 2 is less future proof but at the same time, I
>> don't believe we will see that IP in another chip in the future.
>
> If we were to follow the spirit of "how is it wired" to the letter, you
> should really use 8 supplies, but I appreciate that it can be
> troublesome (or maybe not? it's just 2 dtsi files after all ;-). So
> maybe, as the compatible values explicitly mention the SOC names, you
> just want to hardcode the voltage levels in the driver itself (probably
> as data for the match array)? This of course assume that the reference
> source is internal. Shortly speaking - I believe that you should have
> phandles to regulators or nothing at all there :-) A de-facto-constant
> list of SOC-specific numbers seems the worst option.
The table is a list of virtual reference voltages, if you will so. The
reference voltage is always the same, but some of the inputs have a voltage
divider. From a mathematical point of view you get the same result if you
either divide the input voltage, or multiply the reference voltage. That
said in my opinion the best solution is still to put that table into the
driver and not the devicetree.
- Lars
On 08/22/13 17:51, Pawel Moll wrote:
> On Thu, 2013-08-22 at 07:17 +0100, Jonathan Cameron wrote:
>> I would favour option 2 though some of the discussions going on at the moment about
>> bindings might result in a generic description of this and any other bits of
>> analog front end.
>
> Any link to this discussion? [email protected] doesn't seem to
> be involved (of course I may have simply missed the thread ;-)
Some cc's dropped off the discussion at some point that should have been there
and the topic has 'wandered' somewhat from where it started.
https://lkml.org/lkml/2013/8/21/409
and the couple of messages after that are probably the most relevant.
Speaking of which the devicetree list on here was the old one, I've just switched
to the vger list you suggest above.
>
> Paweł
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Hector,
I'd like to go forward with this and have a chance to see that included
in 3.13. Do you mind if I rework your patch series by removing the DT
part and then resubmit ?
Regards,
On 24/08/2013 01:00, Jonathan Cameron wrote:
> On 08/22/13 17:51, Pawel Moll wrote:
>> On Thu, 2013-08-22 at 07:17 +0100, Jonathan Cameron wrote:
>>> I would favour option 2 though some of the discussions going on at the moment about
>>> bindings might result in a generic description of this and any other bits of
>>> analog front end.
>> Any link to this discussion? [email protected] doesn't seem to
>> be involved (of course I may have simply missed the thread ;-)
> Some cc's dropped off the discussion at some point that should have been there
> and the topic has 'wandered' somewhat from where it started.
>
> https://lkml.org/lkml/2013/8/21/409
>
> and the couple of messages after that are probably the most relevant.
>
> Speaking of which the devicetree list on here was the old one, I've just switched
> to the vger list you suggest above.
>
>
>> Paweł
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi Alexandre,
On 09/23/2013 02:47 PM, Alexandre Belloni wrote:
> Hector,
>
> I'd like to go forward with this and have a chance to see that included
> in 3.13. Do you mind if I rework your patch series by removing the DT
> part and then resubmit ?
I don't mind at all. Please go ahead.
>
> Regards,
>
>
> On 24/08/2013 01:00, Jonathan Cameron wrote:
>> On 08/22/13 17:51, Pawel Moll wrote:
>>> On Thu, 2013-08-22 at 07:17 +0100, Jonathan Cameron wrote:
>>>> I would favour option 2 though some of the discussions going on at the moment about
>>>> bindings might result in a generic description of this and any other bits of
>>>> analog front end.
>>> Any link to this discussion? [email protected] doesn't seem to
>>> be involved (of course I may have simply missed the thread ;-)
>> Some cc's dropped off the discussion at some point that should have been there
>> and the topic has 'wandered' somewhat from where it started.
>>
>> https://lkml.org/lkml/2013/8/21/409
>>
>> and the couple of messages after that are probably the most relevant.
>>
>> Speaking of which the devicetree list on here was the old one, I've just switched
>> to the vger list you suggest above.
>>
>>
>>> Paweł
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>
>
Best regards,
--
Hector Palacios