Greetings,
This is v2 of the patchset that adds support to the optional
divider_by_two of LRADC channels.
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 | 249 ++++++++++++++++++---
4 files changed, 231 insertions(+), 35 deletions(-)
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]>
---
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 d92c97a..91282dc 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
*/
@@ -547,9 +550,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);
@@ -821,7 +825,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, \
}, \
}
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]>.
---
drivers/staging/iio/adc/mxs-lradc.c | 81 ++++++++++++++++++++++++-------------
1 file changed, 52 insertions(+), 29 deletions(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 91282dc..99e5790 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,39 +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,
+static int mxs_lradc_read_single(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;
- unsigned long mask;
-
- if (m != IIO_CHAN_INFO_RAW)
- return -EINVAL;
-
- /* Check for invalid channel */
- if (chan->channel > LRADC_MAX_TOTAL_CHANS)
- return -EINVAL;
-
- /* Validate the channel if it doesn't intersect with reserved chans. */
- bitmap_set(&mask, chan->channel, 1);
- ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
- if (ret)
- 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);
@@ -300,6 +275,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, val2, m);
+ 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;
@@ -821,7 +837,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', \
@@ -960,6 +977,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
Signed-off-by: Hector Palacios <[email protected]>
---
drivers/staging/iio/adc/mxs-lradc.c | 103 +++++++++++++++++++++++++++++++++++-
1 file changed, 102 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index 99e5790..c929733 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>
@@ -142,6 +143,7 @@ struct mxs_lradc {
struct completion completion;
uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
+ unsigned int scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
/*
* Touchscreen LRADC channels receives a private slot in the CTRL4
@@ -321,9 +323,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][0],
+ lradc->scale_avail[ch][i][1]);
+
+ 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,
};
/*
@@ -840,6 +917,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, \
@@ -927,7 +1005,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));
@@ -1006,6 +1085,28 @@ 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.
+ * [0] = Integer part of the scale
+ * [1] = Decimal part of the scale
+ */
+ scale_uv = ((u64)lradc->vref_mv[i] * 100000000) >>
+ (iio->channels[i].scan_type.realbits - s);
+ lradc->scale_avail[i][s][1] = do_div(scale_uv,
+ 100000000) * 10;
+ lradc->scale_avail[i][s][0] = scale_uv;
+ }
+ }
+
/* Configure the hardware. */
mxs_lradc_hw_init(lradc);
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]>
---
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";
};
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 | 55 ++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
index c929733..286cde2 100644
--- a/drivers/staging/iio/adc/mxs-lradc.c
+++ b/drivers/staging/iio/adc/mxs-lradc.c
@@ -144,6 +144,7 @@ struct mxs_lradc {
uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
unsigned int scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
+ unsigned int is_divided[LRADC_MAX_TOTAL_CHANS];
/*
* Touchscreen LRADC channels receives a private slot in the CTRL4
@@ -202,6 +203,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
@@ -310,7 +312,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:
@@ -323,6 +326,54 @@ 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);
+ int ret;
+
+ ret = mutex_trylock(&lradc->lock);
+ if (!ret)
+ return -EBUSY;
+
+ switch (m) {
+ case IIO_CHAN_INFO_SCALE:
+ ret = -EINVAL;
+ if (val == lradc->scale_avail[chan->channel][0][0] &&
+ val2 == lradc->scale_avail[chan->channel][0][1]) {
+ /* [0] -> 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 == lradc->scale_avail[chan->channel][1][0] &&
+ val2 == lradc->scale_avail[chan->channel][1][1]) {
+ /* [1] -> 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,
@@ -400,6 +451,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,
};
Dear Hector Palacios,
> 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]>
Best regards,
Marek Vasut
Dear Hector Palacios,
> 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]>
> ---
> 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"
This is a separate fix, but I think it's not that important to rework the whole
patchset because of it.
Acked-by: Marek Vasut <[email protected]>
Best regards,
Marek Vasut
Dear Hector Palacios,
> 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]>.
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 81
> ++++++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 29
> deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index 91282dc..99e5790 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,39 +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,
> +static int mxs_lradc_read_single(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;
> - unsigned long mask;
> -
> - if (m != IIO_CHAN_INFO_RAW)
> - return -EINVAL;
> -
> - /* Check for invalid channel */
> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
> - return -EINVAL;
This was already resolved, so this patch won't apply I'm afraid.
> - /* Validate the channel if it doesn't intersect with reserved chans. */
> - bitmap_set(&mask, chan->channel, 1);
> - ret = iio_validate_scan_mask_onehot(iio_dev, &mask);
> - if (ret)
> - 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);
>
> @@ -300,6 +275,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, val2, m);
> + 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;
> @@ -821,7 +837,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', \
> @@ -960,6 +977,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);
Otherwise:
Acked-by: Marek Vasut <[email protected]>
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]>
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 55
> ++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1
> deletion(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> b/drivers/staging/iio/adc/mxs-lradc.c index c929733..286cde2 100644
> --- a/drivers/staging/iio/adc/mxs-lradc.c
> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> @@ -144,6 +144,7 @@ struct mxs_lradc {
>
> uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
> unsigned int scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
> + unsigned int is_divided[LRADC_MAX_TOTAL_CHANS];
Why not use bitfield ? ;-)
> /*
> * Touchscreen LRADC channels receives a private slot in the CTRL4
> @@ -202,6 +203,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
> @@ -310,7 +312,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:
> @@ -323,6 +326,54 @@ 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);
> + int ret;
> +
> + ret = mutex_trylock(&lradc->lock);
> + if (!ret)
> + return -EBUSY;
> +
> + switch (m) {
> + case IIO_CHAN_INFO_SCALE:
> + ret = -EINVAL;
> + if (val == lradc->scale_avail[chan->channel][0][0] &&
> + val2 == lradc->scale_avail[chan->channel][0][1]) {
> + /* [0] -> divider by two disabled */
This comment is confusing, you use [0] in different dimensions of the array. So
is the stuff below.
Still, how does this even work, can you show me and example how to set the
divider from userland ? Sorry, I'm a bit confused with this 3D-business here,
even if the comment in previous patch made it a bit clearer. Actually you can
use some #define to specify if you're accessing div/2 or div/1 portion of the
array to make it more readable.
Like ... scale_avail[chan->channel][LRADC_DIV_BY_2][LRADC_DECIMAL_PART] ...
would by nice.
> + 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 == lradc->scale_avail[chan->channel][1][0] &&
> + val2 == lradc->scale_avail[chan->channel][1][1]) {
> + /* [1] -> 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,
> @@ -400,6 +451,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,
Is this & needed here ?
> .attrs = &mxs_lradc_attribute_group,
> };
Best regards,
Marek Vasut
Dear Marek,
On 07/19/2013 04:39 PM, Marek Vasut wrote:
> 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]>
>> ---
>> drivers/staging/iio/adc/mxs-lradc.c | 55
>> ++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1
>> deletion(-)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>> b/drivers/staging/iio/adc/mxs-lradc.c index c929733..286cde2 100644
>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>> @@ -144,6 +144,7 @@ struct mxs_lradc {
>>
>> uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
>> unsigned int scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
>> + unsigned int is_divided[LRADC_MAX_TOTAL_CHANS];
>
> Why not use bitfield ? ;-)
This is used in some math below and an unsigned int looked more appropriate:
case IIO_CHAN_INFO_SCALE:
*val = lradc->vref_mv[chan->channel];
*val2 = chan->scan_type.realbits -
lradc->is_divided[chan->channel];
ret = IIO_VAL_FRACTIONAL_LOG2;
break;
>
>> /*
>> * Touchscreen LRADC channels receives a private slot in the CTRL4
>> @@ -202,6 +203,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
>> @@ -310,7 +312,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:
>> @@ -323,6 +326,54 @@ 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);
>> + int ret;
>> +
>> + ret = mutex_trylock(&lradc->lock);
>> + if (!ret)
>> + return -EBUSY;
>> +
>> + switch (m) {
>> + case IIO_CHAN_INFO_SCALE:
>> + ret = -EINVAL;
>> + if (val == lradc->scale_avail[chan->channel][0][0] &&
>> + val2 == lradc->scale_avail[chan->channel][0][1]) {
>> + /* [0] -> divider by two disabled */
>
> This comment is confusing, you use [0] in different dimensions of the array. So
> is the stuff below.
>
> Still, how does this even work, can you show me and example how to set the
> divider from userland ? Sorry, I'm a bit confused with this 3D-business here,
> even if the comment in previous patch made it a bit clearer. Actually you can
> use some #define to specify if you're accessing div/2 or div/1 portion of the
> array to make it more readable.
>
> Like ... scale_avail[chan->channel][LRADC_DIV_BY_2][LRADC_DECIMAL_PART] ...
> would by nice.
Agreed.
How it works:
# cd /sys/devices/80000000.apb/80040000.apbx/80050000.lradc/iio:device0
Here you have three entries per channel:
in_voltageX_raw -> the sample raw value
in_voltageX_scale -> the scale to multiply the raw value to get the voltage in mV
in_voltageX_scale_available -> lists the available scales of the channel
For example for channel 0:
# cat in_voltage0_scale_available
0.451660156 0.903320312 (two scales available, first with divider by two disabled,
second with divider by two enabled)
# cat in_voltage0_scale
0.451660156 (divider by two is currently disabled)
# cat in_voltage0_raw (shows measured value)
1000
If you multiply the value by the scale you get: 1000 * 0.451660156 = 451.6 mV
# echo 0.903320312 > in_voltage0_scale (enables the divider by two)
# cat in_voltage0_raw (shows measured value)
500
Voltage at channel is the same but now measured value is applying the scale so it
shows half the value than before. Now if you multiply: 500 * 0.903320312 = 451.6 mV
(the same voltage but you now have a bigger scale and can measure up to 3.7V).
Other channels (like 10 on the MX28) will show different scales because of fixed
predividers.
The multi-dimension array is needed to store the big decimal number.
>> + 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 == lradc->scale_avail[chan->channel][1][0] &&
>> + val2 == lradc->scale_avail[chan->channel][1][1]) {
>> + /* [1] -> 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,
>> @@ -400,6 +451,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,
>
> Is this & needed here ?
No it isn't.
Thanks.
Best regards,
--
Hector Palacios
Dear Marek,
On 07/19/2013 04:30 PM, Marek Vasut wrote:
>> @@ -228,39 +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,
>> +static int mxs_lradc_read_single(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;
>> - unsigned long mask;
>> -
>> - if (m != IIO_CHAN_INFO_RAW)
>> - return -EINVAL;
>> -
>> - /* Check for invalid channel */
>> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
>> - return -EINVAL;
>
> This was already resolved, so this patch won't apply I'm afraid.
You mean the 'unsigned long mask', right? Yeah, I think I had resolved that one
before submitting, but looks like I didn't.
The other check is not resolved afaik. We agreed to remove it, but on a different patch.
> Otherwise:
>
> Acked-by: Marek Vasut <[email protected]>
>
>
> Marek Vasut
>
Best regards,
--
Hector Palacios
Dear Hector Palacios,
> Dear Marek,
>
> On 07/19/2013 04:30 PM, Marek Vasut wrote:
> >> @@ -228,39 +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,
> >> +static int mxs_lradc_read_single(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;
> >>
> >> - unsigned long mask;
> >> -
> >> - if (m != IIO_CHAN_INFO_RAW)
> >> - return -EINVAL;
> >> -
> >> - /* Check for invalid channel */
> >> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
> >> - return -EINVAL;
> >
> > This was already resolved, so this patch won't apply I'm afraid.
>
> You mean the 'unsigned long mask', right? Yeah, I think I had resolved
> that one before submitting, but looks like I didn't.
> The other check is not resolved afaik. We agreed to remove it, but on a
> different patch.
I mean the other check, yeah. A patch removing that should be applied already.
Best regards,
Marek Vasut
Dear Hector Palacios,
> Dear Marek,
>
> On 07/19/2013 04:39 PM, Marek Vasut wrote:
> > 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]>
> >> ---
> >>
> >> drivers/staging/iio/adc/mxs-lradc.c | 55
> >>
> >> ++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+),
> >> 1 deletion(-)
> >>
> >> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
> >> b/drivers/staging/iio/adc/mxs-lradc.c index c929733..286cde2 100644
> >> --- a/drivers/staging/iio/adc/mxs-lradc.c
> >> +++ b/drivers/staging/iio/adc/mxs-lradc.c
> >> @@ -144,6 +144,7 @@ struct mxs_lradc {
> >>
> >> uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
> >> unsigned int scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
> >>
> >> + unsigned int is_divided[LRADC_MAX_TOTAL_CHANS];
> >
> > Why not use bitfield ? ;-)
>
> This is used in some math below and an unsigned int looked more
> appropriate:
>
> case IIO_CHAN_INFO_SCALE:
> *val = lradc->vref_mv[chan->channel];
> *val2 = chan->scan_type.realbits -
> lradc->is_divided[chan->channel];
> ret = IIO_VAL_FRACTIONAL_LOG2;
> break;
Oh ok.
> >> /*
> >>
> >> * Touchscreen LRADC channels receives a private slot in the CTRL4
> >>
> >> @@ -202,6 +203,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
> >>
> >> @@ -310,7 +312,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:
> >> @@ -323,6 +326,54 @@ 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);
> >> + int ret;
> >> +
> >> + ret = mutex_trylock(&lradc->lock);
> >> + if (!ret)
> >> + return -EBUSY;
> >> +
> >> + switch (m) {
> >> + case IIO_CHAN_INFO_SCALE:
> >> + ret = -EINVAL;
> >> + if (val == lradc->scale_avail[chan->channel][0][0] &&
> >> + val2 == lradc->scale_avail[chan->channel][0][1]) {
> >> + /* [0] -> divider by two disabled */
> >
> > This comment is confusing, you use [0] in different dimensions of the
> > array. So is the stuff below.
> >
> > Still, how does this even work, can you show me and example how to set
> > the divider from userland ? Sorry, I'm a bit confused with this
> > 3D-business here, even if the comment in previous patch made it a bit
> > clearer. Actually you can use some #define to specify if you're
> > accessing div/2 or div/1 portion of the array to make it more readable.
> >
> > Like ... scale_avail[chan->channel][LRADC_DIV_BY_2][LRADC_DECIMAL_PART]
> > ... would by nice.
>
> Agreed.
>
> How it works:
> # cd /sys/devices/80000000.apb/80040000.apbx/80050000.lradc/iio:device0
>
> Here you have three entries per channel:
> in_voltageX_raw -> the sample raw value
> in_voltageX_scale -> the scale to multiply the raw value to get the
voltage
> in mV in_voltageX_scale_available -> lists the available scales of the
> channel
>
> For example for channel 0:
>
> # cat in_voltage0_scale_available
> 0.451660156 0.903320312 (two scales available, first with divider by two
> disabled, second with divider by two enabled)
>
> # cat in_voltage0_scale
> 0.451660156 (divider by two is currently disabled)
>
> # cat in_voltage0_raw (shows measured value)
> 1000
>
> If you multiply the value by the scale you get: 1000 * 0.451660156 = 451.6
> mV
>
> # echo 0.903320312 > in_voltage0_scale (enables the divider by two)
Ok, so I have to remember this value : '0.903320312' in case I want to enable
divide-by-two functionality? Hmmmm ... why would this interface not work:
echo 2 > in_voltage0_scale
or
echo 1 > in_voltage0_scale
?
> # cat in_voltage0_raw (shows measured value)
> 500
>
> Voltage at channel is the same but now measured value is applying the scale
> so it shows half the value than before. Now if you multiply: 500 *
> 0.903320312 = 451.6 mV (the same voltage but you now have a bigger scale
> and can measure up to 3.7V).
>
> Other channels (like 10 on the MX28) will show different scales because of
> fixed predividers.
> The multi-dimension array is needed to store the big decimal number.
Yes, understood. Thanks for the explanation, it really helped!
Dear Marek,
On 07/19/2013 06:17 PM, Marek Vasut wrote:
>> Here you have three entries per channel:
>> in_voltageX_raw -> the sample raw value
>> in_voltageX_scale -> the scale to multiply the raw value to get the
> voltage
>> in mV in_voltageX_scale_available -> lists the available scales of the
>> channel
>>
>> For example for channel 0:
>>
>> # cat in_voltage0_scale_available
>> 0.451660156 0.903320312 (two scales available, first with divider by two
>> disabled, second with divider by two enabled)
>>
>> # cat in_voltage0_scale
>> 0.451660156 (divider by two is currently disabled)
>>
>> # cat in_voltage0_raw (shows measured value)
>> 1000
>>
>> If you multiply the value by the scale you get: 1000 * 0.451660156 = 451.6
>> mV
>>
>> # echo 0.903320312 > in_voltage0_scale (enables the divider by two)
>
> Ok, so I have to remember this value : '0.903320312' in case I want to enable
> divide-by-two functionality?
No you don't. That's why there is a 'in_voltage_scale_available' that lists the
available values.
> Hmmmm ... why would this interface not work:
> echo 2 > in_voltage0_scale
>
> or
>
> echo 1 > in_voltage0_scale
An easy thing like that is what I first submitted, but it was rejected and I was told
to use the scale_available descriptor instead, which is the common interface the rest
of drivers use.
Best regards,
--
Hector Palacios
Hi Hector,
On 19/07/2013 11:13, Hector Palacios wrote:
> 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]>.
> ---
> drivers/staging/iio/adc/mxs-lradc.c | 81 ++++++++++++++++++++++++-------------
> 1 file changed, 52 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
> index 91282dc..99e5790 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,39 +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,
> +static int mxs_lradc_read_single(struct iio_dev *iio_dev,
> const struct iio_chan_spec *chan,
> int *val, int *val2, long m)
You don't need val2 and m in that function, I woud suggest no passing
those. Also, in my patch, I was passing channel->chan as an int but I
don't see any drawbacks of passing a pointer to the struct iio_chan_spec.
As a note, I'm waiting for your patch to get included so that we won't
get any conflicts.
Regards,
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 19/07/2013 11:13, 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]>
> ---
>
Looks good to me:
Acked-by: Alexandre Belloni <[email protected]>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 19/07/2013 11:13, 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: Alexandre Belloni <[email protected]>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 19/07/2013 11:13, Hector Palacios wrote:
> 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
>
> Signed-off-by: Hector Palacios <[email protected]>
Looks good to me:
Acked-by: Alexandre Belloni <[email protected]>
And you are right, we will probably need to hide the 64bits math. I'll
try to think of something.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 19/07/2013 11:13, Hector Palacios wrote:
> 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]>
Looks good to me:
Acked-by: Alexandre Belloni <[email protected]>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On 07/19/2013 05:22 PM, Hector Palacios wrote:
> Dear Marek,
>
> On 07/19/2013 06:17 PM, Marek Vasut wrote:
>>> Here you have three entries per channel:
>>> in_voltageX_raw -> the sample raw value
>>> in_voltageX_scale -> the scale to multiply the raw value to get the
>> voltage
>>> in mV in_voltageX_scale_available -> lists the available scales of the
>>> channel
>>>
>>> For example for channel 0:
>>>
>>> # cat in_voltage0_scale_available
>>> 0.451660156 0.903320312 (two scales available, first with divider by two
>>> disabled, second with divider by two enabled)
>>>
>>> # cat in_voltage0_scale
>>> 0.451660156 (divider by two is currently disabled)
>>>
>>> # cat in_voltage0_raw (shows measured value)
>>> 1000
>>>
>>> If you multiply the value by the scale you get: 1000 * 0.451660156 = 451.6
>>> mV
>>>
>>> # echo 0.903320312 > in_voltage0_scale (enables the divider by two)
>>
>> Ok, so I have to remember this value : '0.903320312' in case I want to enable
>> divide-by-two functionality?
>
> No you don't. That's why there is a 'in_voltage_scale_available' that lists the available values.
>
>> Hmmmm ... why would this interface not work:
>> echo 2 > in_voltage0_scale
>>
>> or
>>
>> echo 1 > in_voltage0_scale
>
> An easy thing like that is what I first submitted, but it was rejected and I was told to use the scale_available
> descriptor instead, which is the common interface the rest of drivers use.
This comes down to allowing us to have one generic predictable interface (which sometimes
ends up looking uggly!). The key thing is that if you are outputing using the _raw
sysfs interfaces, the aim is to avoid doing nasty maths in kernel to get to 'standard' units such as mV.
Hence that scale attribute tells you what to apply to the value. If you just wanted it
to be 1 or 2 then the in_voltage0_raw value would have to be a long and nasty
decimal. Now we do have the option of in_voltage0_calibscale which would be applied
internally to the value but it really isn't for this purpose (it's for devices with a 'trim'
control) and you'd still have scale set to 0.903320312 or similar. Although they have
meaning obviously, in this case 1 and 2 are little more than 'magic' numbers.
Note that when things are quite, I'm at least in theory working on a cleaner interface
for these 'available' attributes that would also provide in kernel access for IIO consumers.
Basically this will be another callback to get either the 'range' of pseudo continuous
variables or the 'available' set for parameters like this.
Being lazy I'm happy to let someone else clean this corner up if they like? *looks hopeful*
Jonathan
>
> Best regards,
> --
> Hector Palacios
On 07/19/2013 04:31 PM, Hector Palacios wrote:
> Dear Marek,
>
> On 07/19/2013 04:39 PM, Marek Vasut wrote:
>> 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]>
>>> ---
>>> drivers/staging/iio/adc/mxs-lradc.c | 55
>>> ++++++++++++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1
>>> deletion(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c
>>> b/drivers/staging/iio/adc/mxs-lradc.c index c929733..286cde2 100644
>>> --- a/drivers/staging/iio/adc/mxs-lradc.c
>>> +++ b/drivers/staging/iio/adc/mxs-lradc.c
>>> @@ -144,6 +144,7 @@ struct mxs_lradc {
>>>
>>> uint32_t vref_mv[LRADC_MAX_TOTAL_CHANS];
>>> unsigned int scale_avail[LRADC_MAX_TOTAL_CHANS][2][2];
>>> + unsigned int is_divided[LRADC_MAX_TOTAL_CHANS];
>>
>> Why not use bitfield ? ;-)
>
> This is used in some math below and an unsigned int looked more appropriate:
>
> case IIO_CHAN_INFO_SCALE:
> *val = lradc->vref_mv[chan->channel];
> *val2 = chan->scan_type.realbits -
> lradc->is_divided[chan->channel];
> ret = IIO_VAL_FRACTIONAL_LOG2;
> break;
>
>>
>>> /*
>>> * Touchscreen LRADC channels receives a private slot in the CTRL4
>>> @@ -202,6 +203,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
>>> @@ -310,7 +312,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:
>>> @@ -323,6 +326,54 @@ 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);
>>> + int ret;
>>> +
>>> + ret = mutex_trylock(&lradc->lock);
>>> + if (!ret)
>>> + return -EBUSY;
>>> +
>>> + switch (m) {
>>> + case IIO_CHAN_INFO_SCALE:
>>> + ret = -EINVAL;
>>> + if (val == lradc->scale_avail[chan->channel][0][0] &&
>>> + val2 == lradc->scale_avail[chan->channel][0][1]) {
>>> + /* [0] -> divider by two disabled */
>>
>> This comment is confusing, you use [0] in different dimensions of the array. So
>> is the stuff below.
>>
>> Still, how does this even work, can you show me and example how to set the
>> divider from userland ? Sorry, I'm a bit confused with this 3D-business here,
>> even if the comment in previous patch made it a bit clearer. Actually you can
>> use some #define to specify if you're accessing div/2 or div/1 portion of the
>> array to make it more readable.
>>
>> Like ... scale_avail[chan->channel][LRADC_DIV_BY_2][LRADC_DECIMAL_PART] ...
>> would by nice.
>
> Agreed.
Could even make the int + nano part a structure then you could have
scale_avail[chan->channel][LRADC_DIV_BY_2].integer / .nano
might not be worth the hassel though for the slight gain in readability.
I'm happy either way.
>
> How it works:
> # cd /sys/devices/80000000.apb/80040000.apbx/80050000.lradc/iio:device0
>
> Here you have three entries per channel:
> in_voltageX_raw -> the sample raw value
> in_voltageX_scale -> the scale to multiply the raw value to get the voltage in mV
> in_voltageX_scale_available -> lists the available scales of the channel
>
> For example for channel 0:
>
> # cat in_voltage0_scale_available
> 0.451660156 0.903320312 (two scales available, first with divider by two disabled, second with divider by two enabled)
>
> # cat in_voltage0_scale
> 0.451660156 (divider by two is currently disabled)
>
> # cat in_voltage0_raw (shows measured value)
> 1000
>
> If you multiply the value by the scale you get: 1000 * 0.451660156 = 451.6 mV
>
> # echo 0.903320312 > in_voltage0_scale (enables the divider by two)
>
> # cat in_voltage0_raw (shows measured value)
> 500
>
> Voltage at channel is the same but now measured value is applying the scale so it shows half the value than before. Now
> if you multiply: 500 * 0.903320312 = 451.6 mV (the same voltage but you now have a bigger scale and can measure up to
> 3.7V).
>
> Other channels (like 10 on the MX28) will show different scales because of fixed predividers.
> The multi-dimension array is needed to store the big decimal number.
>
>>> + 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 == lradc->scale_avail[chan->channel][1][0] &&
>>> + val2 == lradc->scale_avail[chan->channel][1][1]) {
>>> + /* [1] -> 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,
>>> @@ -400,6 +451,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,
>>
>> Is this & needed here ?
>
> No it isn't.
> Thanks.
>
> Best regards,
> --
> Hector Palacios
> --
> 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 Jonathan Cameron,
> On 07/19/2013 05:22 PM, Hector Palacios wrote:
> > Dear Marek,
> >
> > On 07/19/2013 06:17 PM, Marek Vasut wrote:
> >>> Here you have three entries per channel:
> >>> in_voltageX_raw -> the sample raw value
> >>> in_voltageX_scale -> the scale to multiply the raw value to get the
> >>
> >> voltage
> >>
> >>> in mV in_voltageX_scale_available -> lists the available scales of the
> >>> channel
> >>>
> >>> For example for channel 0:
> >>>
> >>> # cat in_voltage0_scale_available
> >>> 0.451660156 0.903320312 (two scales available, first with divider by
> >>> two disabled, second with divider by two enabled)
> >>>
> >>> # cat in_voltage0_scale
> >>> 0.451660156 (divider by two is currently disabled)
> >>>
> >>> # cat in_voltage0_raw (shows measured value)
> >>> 1000
> >>>
> >>> If you multiply the value by the scale you get: 1000 * 0.451660156 =
> >>> 451.6 mV
> >>>
> >>> # echo 0.903320312 > in_voltage0_scale (enables the divider by two)
> >>
> >> Ok, so I have to remember this value : '0.903320312' in case I want to
> >> enable divide-by-two functionality?
> >
> > No you don't. That's why there is a 'in_voltage_scale_available' that
> > lists the available values.
> >
> >> Hmmmm ... why would this interface not work:
> >> echo 2 > in_voltage0_scale
> >>
> >> or
> >>
> >> echo 1 > in_voltage0_scale
> >
> > An easy thing like that is what I first submitted, but it was rejected
> > and I was told to use the scale_available descriptor instead, which is
> > the common interface the rest of drivers use.
>
> This comes down to allowing us to have one generic predictable interface
> (which sometimes ends up looking uggly!). The key thing is that if you
> are outputing using the _raw sysfs interfaces, the aim is to avoid doing
> nasty maths in kernel to get to 'standard' units such as mV.
OK, understood.
> Hence that scale attribute tells you what to apply to the value. If you
> just wanted it to be 1 or 2 then the in_voltage0_raw value would have to
> be a long and nasty decimal. Now we do have the option of
> in_voltage0_calibscale which would be applied internally to the value but
> it really isn't for this purpose (it's for devices with a 'trim' control)
> and you'd still have scale set to 0.903320312 or similar. Although they
> have meaning obviously, in this case 1 and 2 are little more than 'magic'
> numbers.
>
> Note that when things are quite, I'm at least in theory working on a
> cleaner interface for these 'available' attributes that would also provide
> in kernel access for IIO consumers. Basically this will be another
> callback to get either the 'range' of pseudo continuous variables or the
> 'available' set for parameters like this.
Thanks for the explanation!
> Being lazy I'm happy to let someone else clean this corner up if they like?
> *looks hopeful*
Please don't look at me, I already am fully loaded with fixing my mess all
around ;-/
Best regards,
Marek Vasut
Hi Marek,
On 07/19/2013 06:14 PM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Dear Marek,
>>
>> On 07/19/2013 04:30 PM, Marek Vasut wrote:
>>>> @@ -228,39 +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,
>>>> +static int mxs_lradc_read_single(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;
>>>>
>>>> - unsigned long mask;
>>>> -
>>>> - if (m != IIO_CHAN_INFO_RAW)
>>>> - return -EINVAL;
>>>> -
>>>> - /* Check for invalid channel */
>>>> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
>>>> - return -EINVAL;
>>>
>>> This was already resolved, so this patch won't apply I'm afraid.
>>
>> You mean the 'unsigned long mask', right? Yeah, I think I had resolved
>> that one before submitting, but looks like I didn't.
>> The other check is not resolved afaik. We agreed to remove it, but on a
>> different patch.
>
> I mean the other check, yeah. A patch removing that should be applied already.
Where exactly? It's not fixed in Jonathan's fixes-togreg branch, at least.
Did you fixed it?
Best regards,
--
Hector Palacios
Hi Alexandre,
On 07/19/2013 07:06 PM, Alexandre Belloni wrote:
> Hi Hector,
>
> On 19/07/2013 11:13, Hector Palacios wrote:
>> 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]>.
>> ---
>> drivers/staging/iio/adc/mxs-lradc.c | 81 ++++++++++++++++++++++++-------------
>> 1 file changed, 52 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/mxs-lradc.c b/drivers/staging/iio/adc/mxs-lradc.c
>> index 91282dc..99e5790 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,39 +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,
>> +static int mxs_lradc_read_single(struct iio_dev *iio_dev,
>> const struct iio_chan_spec *chan,
>> int *val, int *val2, long m)
> You don't need val2 and m in that function, I woud suggest no passing
> those. Also, in my patch, I was passing channel->chan as an int but I
> don't see any drawbacks of passing a pointer to the struct iio_chan_spec.
Ok, I'll remove val2 and m and keep the pointer.
> As a note, I'm waiting for your patch to get included so that we won't
> get any conflicts.
I know, thank you for that.
Best regards,
--
Hector Palacios
Dear Hector Palacios,
> Hi Marek,
>
> On 07/19/2013 06:14 PM, Marek Vasut wrote:
> > Dear Hector Palacios,
> >
> >> Dear Marek,
> >>
> >> On 07/19/2013 04:30 PM, Marek Vasut wrote:
> >>>> @@ -228,39 +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,
> >>>> +static int mxs_lradc_read_single(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;
> >>>>
> >>>> - unsigned long mask;
> >>>> -
> >>>> - if (m != IIO_CHAN_INFO_RAW)
> >>>> - return -EINVAL;
> >>>> -
> >>>> - /* Check for invalid channel */
> >>>> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
> >>>> - return -EINVAL;
> >>>
> >>> This was already resolved, so this patch won't apply I'm afraid.
> >>
> >> You mean the 'unsigned long mask', right? Yeah, I think I had resolved
> >> that one before submitting, but looks like I didn't.
> >> The other check is not resolved afaik. We agreed to remove it, but on a
> >> different patch.
> >
> > I mean the other check, yeah. A patch removing that should be applied
> > already.
>
> Where exactly? It's not fixed in Jonathan's fixes-togreg branch, at least.
> Did you fixed it?
I use linux-next [1], should be it.
http://git.kernel.org/cgit/linux/kernel/git/next/linux-
next.git/log/drivers/staging/iio/adc/mxs-lradc.c
Best regards,
Marek Vasut
Hi Marek,
On 07/22/2013 09:42 AM, Marek Vasut wrote:
> Dear Hector Palacios,
>
>> Hi Marek,
>>
>> On 07/19/2013 06:14 PM, Marek Vasut wrote:
>>> Dear Hector Palacios,
>>>
>>>> Dear Marek,
>>>>
>>>> On 07/19/2013 04:30 PM, Marek Vasut wrote:
>>>>>> @@ -228,39 +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,
>>>>>> +static int mxs_lradc_read_single(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;
>>>>>>
>>>>>> - unsigned long mask;
>>>>>> -
>>>>>> - if (m != IIO_CHAN_INFO_RAW)
>>>>>> - return -EINVAL;
>>>>>> -
>>>>>> - /* Check for invalid channel */
>>>>>> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
>>>>>> - return -EINVAL;
>>>>>
>>>>> This was already resolved, so this patch won't apply I'm afraid.
>>>>
>>>> You mean the 'unsigned long mask', right? Yeah, I think I had resolved
>>>> that one before submitting, but looks like I didn't.
>>>> The other check is not resolved afaik. We agreed to remove it, but on a
>>>> different patch.
>>>
>>> I mean the other check, yeah. A patch removing that should be applied
>>> already.
>>
>> Where exactly? It's not fixed in Jonathan's fixes-togreg branch, at least.
>> Did you fixed it?
>
> I use linux-next [1], should be it.
>
> http://git.kernel.org/cgit/linux/kernel/git/next/linux-
> next.git/log/drivers/staging/iio/adc/mxs-lradc.c
That is removing the unsigned long mask, but not the check for invalid channel.
I'm taking care of the unsigned long mask but an additional patch is needed to remove
the check for invalid channel.
Best regards,
--
Hector Palacios
Hi Hector,
> Hi Marek,
>
> On 07/22/2013 09:42 AM, Marek Vasut wrote:
> > Dear Hector Palacios,
> >
> >> Hi Marek,
> >>
> >> On 07/19/2013 06:14 PM, Marek Vasut wrote:
> >>> Dear Hector Palacios,
> >>>
> >>>> Dear Marek,
> >>>>
> >>>> On 07/19/2013 04:30 PM, Marek Vasut wrote:
> >>>>>> @@ -228,39 +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,
> >>>>>> +static int mxs_lradc_read_single(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;
> >>>>>>
> >>>>>> - unsigned long mask;
> >>>>>> -
> >>>>>> - if (m != IIO_CHAN_INFO_RAW)
> >>>>>> - return -EINVAL;
> >>>>>> -
> >>>>>> - /* Check for invalid channel */
> >>>>>> - if (chan->channel > LRADC_MAX_TOTAL_CHANS)
> >>>>>> - return -EINVAL;
> >>>>>
> >>>>> This was already resolved, so this patch won't apply I'm afraid.
> >>>>
> >>>> You mean the 'unsigned long mask', right? Yeah, I think I had
> >>>> resolved that one before submitting, but looks like I didn't.
> >>>> The other check is not resolved afaik. We agreed to remove it, but on
> >>>> a different patch.
> >>>
> >>> I mean the other check, yeah. A patch removing that should be applied
> >>> already.
> >>
> >> Where exactly? It's not fixed in Jonathan's fixes-togreg branch, at
> >> least. Did you fixed it?
> >
> > I use linux-next [1], should be it.
> >
> > http://git.kernel.org/cgit/linux/kernel/git/next/linux-
> > next.git/log/drivers/staging/iio/adc/mxs-lradc.c
>
> That is removing the unsigned long mask, but not the check for invalid
> channel. I'm taking care of the unsigned long mask but an additional patch
> is needed to remove the check for invalid channel.
Sorry for the confusion.
Best regards,
Marek Vasut
Hi Jonathan,
On 07/19/2013 10:32 PM, Jonathan Cameron wrote:
>>>> @@ -323,6 +326,54 @@ 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);
>>>> + int ret;
>>>> +
>>>> + ret = mutex_trylock(&lradc->lock);
>>>> + if (!ret)
>>>> + return -EBUSY;
>>>> +
>>>> + switch (m) {
>>>> + case IIO_CHAN_INFO_SCALE:
>>>> + ret = -EINVAL;
>>>> + if (val == lradc->scale_avail[chan->channel][0][0] &&
>>>> + val2 == lradc->scale_avail[chan->channel][0][1]) {
>>>> + /* [0] -> divider by two disabled */
>>>
>>> This comment is confusing, you use [0] in different dimensions of the array. So
>>> is the stuff below.
>>>
>>> Still, how does this even work, can you show me and example how to set the
>>> divider from userland ? Sorry, I'm a bit confused with this 3D-business here,
>>> even if the comment in previous patch made it a bit clearer. Actually you can
>>> use some #define to specify if you're accessing div/2 or div/1 portion of the
>>> array to make it more readable.
>>>
>>> Like ... scale_avail[chan->channel][LRADC_DIV_BY_2][LRADC_DECIMAL_PART] ...
>>> would by nice.
>>
>> Agreed.
> Could even make the int + nano part a structure then you could have
> scale_avail[chan->channel][LRADC_DIV_BY_2].integer / .nano
>
> might not be worth the hassel though for the slight gain in readability.
>
> I'm happy either way.
I prefer the struct approach, it removes one dimension to the array and I find it cleaner.
Best regards,
--
Hector Palacios