* [PATCH v3 2/5] iio: ad7949: Support internal Vref
2019-05-13 14:53 [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Adam Michaelis
@ 2019-05-13 14:53 ` Adam Michaelis
2019-05-18 9:07 ` Jonathan Cameron
2019-05-23 12:12 ` Alexandru Ardelean
2019-05-13 14:53 ` [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select Adam Michaelis
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Adam Michaelis @ 2019-05-13 14:53 UTC (permalink / raw)
To: linux-iio
Cc: lars, michael.hennerich, jic23, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell, Adam Michaelis
Adding configurable (via device tree) options to select one of the two
external reference voltages (REFIN as default, original implementation)
or one of the two internal reference voltages provided by the AD7949
part family.
Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
V2:
- Add some defines to reduce use of magic numbers.
V3:
- Add bitfield.h macros throughout.
- Re-think usage of device tree parameter to focus on the
actual reference sources instead of the raw hardware
configuration.
---
drivers/iio/adc/ad7949.c | 138 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 111 insertions(+), 27 deletions(-)
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index c7fe27aa2519..b648b1ab9559 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,12 +11,23 @@
#include <linux/module.h>
#include <linux/regulator/consumer.h>
#include <linux/spi/spi.h>
-
-#define AD7949_MASK_CHANNEL_SEL GENMASK(9, 7)
-#define AD7949_MASK_TOTAL GENMASK(13, 0)
-#define AD7949_OFFSET_CHANNEL_SEL 7
-#define AD7949_CFG_READ_BACK 0x1
-#define AD7949_CFG_REG_SIZE_BITS 14
+#include <linux/of.h>
+#include <linux/bitfield.h>
+
+#define AD7949_CFG_REG_SIZE_BITS 14
+#define AD7949_CFG_MASK_TOTAL GENMASK(13, 0)
+#define AD7949_CFG_APPLY BIT(13)
+#define AD7949_CFG_CHAN_CFG GENMASK(12, 10)
+#define AD7949_CFG_CHAN_CFG_UNIPOLAR_GND 0x7
+#define AD7949_CFG_CHAN_SEL GENMASK(9, 7)
+#define AD7949_CFG_BW BIT(6)
+#define AD7949_CFG_BW_FULL 1
+#define AD7949_CFG_REF_SEL GENMASK(5, 3)
+#define AD7949_CFG_SEQ GENMASK(2, 1)
+#define AD7949_CFG_SEQ_DISABLED 0x0
+#define AD7949_CFG_READBACK BIT(0)
+#define AD7949_CFG_READBACK_EN 0
+#define AD7949_CFG_READBACK_DIS 1
enum {
ID_AD7949 = 0,
@@ -24,6 +35,18 @@ enum {
ID_AD7689,
};
+enum ad7949_ref_sel {
+ AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
+ AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
+ AD7949_REF_EXT_TEMP, /* REF + temp sensor */
+ AD7949_REF_EXT_TEMP_BUF, /* REFIN + temp sensor */
+ AD7949_REF_RSRV_4,
+ AD7949_REF_RSRV_5,
+ AD7949_REF_EXT, /* REF, no temp */
+ AD7949_REF_EXT_BUF, /* REFIN, no temp */
+ AD7949_REF_MAX,
+};
+
struct ad7949_adc_spec {
u8 num_channels;
u8 resolution;
@@ -41,6 +64,7 @@ struct ad7949_adc_spec {
* @vref: regulator generating Vref
* @iio_dev: reference to iio structure
* @spi: reference to spi structure
+ * @ref_sel: selected reference voltage source
* @resolution: resolution of the chip
* @cfg: copy of the configuration register
* @current_channel: current channel in use
@@ -51,6 +75,7 @@ struct ad7949_adc_chip {
struct regulator *vref;
struct iio_dev *indio_dev;
struct spi_device *spi;
+ enum ad7949_ref_sel ref_sel;
u8 resolution;
u16 cfg;
unsigned int current_channel;
@@ -59,7 +84,7 @@ struct ad7949_adc_chip {
static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
{
- if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
+ if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
return true;
return false;
@@ -91,7 +116,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
};
ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
- ad7949_adc->buffer = ad7949_adc->cfg << shift;
+ ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
spi_message_init_with_transfers(&msg, tx, 1);
ret = spi_sync(ad7949_adc->spi, &msg);
@@ -136,8 +161,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
};
ret = ad7949_spi_write_cfg(ad7949_adc,
- channel << AD7949_OFFSET_CHANNEL_SEL,
- AD7949_MASK_CHANNEL_SEL);
+ FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
+ AD7949_CFG_CHAN_SEL);
if (ret)
return ret;
@@ -204,11 +229,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
- ret = regulator_get_voltage(ad7949_adc->vref);
- if (ret < 0)
- return ret;
+ if (ad7949_adc->vref) {
+ ret = regulator_get_voltage(ad7949_adc->vref);
+ if (ret < 0)
+ return ret;
+
+ *val = ret / 5000;
+ } else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
+ *val = 2500;
+ } else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
+ *val = 4096;
+ } else {
+ return -EINVAL;
+ }
- *val = ret / 5000;
return IIO_VAL_INT;
}
@@ -226,7 +260,8 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
*readval = ad7949_adc->cfg;
else
ret = ad7949_spi_write_cfg(ad7949_adc,
- writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
+ writeval & AD7949_CFG_MASK_TOTAL,
+ AD7949_CFG_MASK_TOTAL);
return ret;
}
@@ -240,10 +275,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
{
int ret;
int val;
+ u16 adc_config = 0;
- /* Sequencer disabled, CFG readback disabled, IN0 as default channel */
ad7949_adc->current_channel = 0;
- ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+ ad7949_adc->cfg = 0;
+
+ adc_config |= FIELD_PREP(AD7949_CFG_APPLY, 1);
+ adc_config |= FIELD_PREP(AD7949_CFG_CHAN_CFG,
+ AD7949_CFG_CHAN_CFG_UNIPOLAR_GND);
+ adc_config |= FIELD_PREP(AD7949_CFG_CHAN_SEL,
+ ad7949_adc->current_channel);
+ adc_config |= FIELD_PREP(AD7949_CFG_BW, AD7949_CFG_BW_FULL);
+ adc_config |= FIELD_PREP(AD7949_CFG_REF_SEL, ad7949_adc->ref_sel);
+ adc_config |= FIELD_PREP(AD7949_CFG_SEQ, AD7949_CFG_SEQ_DISABLED);
+ adc_config |= FIELD_PREP(AD7949_CFG_READBACK, AD7949_CFG_READBACK_DIS);
+
+ ret = ad7949_spi_write_cfg(ad7949_adc,
+ adc_config,
+ AD7949_CFG_MASK_TOTAL);
/*
* Do a dummy conversion to apply the first configuration setting.
@@ -261,6 +310,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
struct ad7949_adc_chip *ad7949_adc;
struct iio_dev *indio_dev;
int ret;
+ u32 temp;
indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
if (!indio_dev) {
@@ -279,21 +329,53 @@ static int ad7949_spi_probe(struct spi_device *spi)
ad7949_adc = iio_priv(indio_dev);
ad7949_adc->indio_dev = indio_dev;
ad7949_adc->spi = spi;
+ ad7949_adc->vref = NULL;
spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
indio_dev->num_channels = spec->num_channels;
ad7949_adc->resolution = spec->resolution;
- ad7949_adc->vref = devm_regulator_get(dev, "vref");
- if (IS_ERR(ad7949_adc->vref)) {
- dev_err(dev, "fail to request regulator\n");
- return PTR_ERR(ad7949_adc->vref);
+ ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
+ "adi,reference-select",
+ &temp);
+ if (ret == 0) {
+ switch (temp) {
+ case 0:
+ ad7949_adc->ref_sel = AD7949_REF_2V5;
+ break;
+ case 1:
+ ad7949_adc->ref_sel = AD7949_REF_4V0;
+ break;
+ case 2:
+ ad7949_adc->ref_sel = AD7949_REF_EXT;
+ break;
+ case 3:
+ ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
+ break;
+ default:
+ ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
+ dev_warn(dev,
+ "unknown reference-select value, using REFIN external Vref (3) by default\n");
+ }
+ } else {
+ ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
+ dev_warn(dev, "using external Vref by default\n");
}
- ret = regulator_enable(ad7949_adc->vref);
- if (ret < 0) {
- dev_err(dev, "fail to enable regulator\n");
- return ret;
+ /* Check whether using external Vref */
+ if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
+ (ad7949_adc->ref_sel != AD7949_REF_4V0)) {
+ ad7949_adc->vref = devm_regulator_get(dev, "vref");
+ if (IS_ERR(ad7949_adc->vref)) {
+ dev_err(dev, "fail to request regulator\n");
+ return PTR_ERR(ad7949_adc->vref);
+ }
+
+ ret = regulator_enable(ad7949_adc->vref);
+ if (ret < 0) {
+ dev_err(dev, "fail to enable regulator\n");
+ return ret;
+ }
}
mutex_init(&ad7949_adc->lock);
@@ -314,7 +396,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
err:
mutex_destroy(&ad7949_adc->lock);
- regulator_disable(ad7949_adc->vref);
+ if (ad7949_adc->vref)
+ regulator_disable(ad7949_adc->vref);
return ret;
}
@@ -326,7 +409,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
iio_device_unregister(indio_dev);
mutex_destroy(&ad7949_adc->lock);
- regulator_disable(ad7949_adc->vref);
+ if (ad7949_adc->vref)
+ regulator_disable(ad7949_adc->vref);
return 0;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/5] iio: ad7949: Support internal Vref
2019-05-13 14:53 ` [PATCH v3 2/5] iio: ad7949: Support internal Vref Adam Michaelis
@ 2019-05-18 9:07 ` Jonathan Cameron
2019-05-23 12:12 ` Alexandru Ardelean
1 sibling, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2019-05-18 9:07 UTC (permalink / raw)
To: Adam Michaelis
Cc: linux-iio, lars, michael.hennerich, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell
On Mon, 13 May 2019 09:53:02 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> Adding configurable (via device tree) options to select one of the two
> external reference voltages (REFIN as default, original implementation)
> or one of the two internal reference voltages provided by the AD7949
> part family.
>
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
Hi Adam,
One comment inline but it's trivial.
I'd not realised before that the external buffered and unbuffered
references were on different pins. One possibility would be
to use different regulator names for each of them, but that is
probably an unnecessary and perhaps confusing step.
So in conclusion I'm happy with this approach
After an Analog review though before applying this.
For personal reference if there should be another version.
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Jonathan
> ---
> V2:
> - Add some defines to reduce use of magic numbers.
> V3:
> - Add bitfield.h macros throughout.
> - Re-think usage of device tree parameter to focus on the
> actual reference sources instead of the raw hardware
> configuration.
> ---
> drivers/iio/adc/ad7949.c | 138 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 111 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index c7fe27aa2519..b648b1ab9559 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -11,12 +11,23 @@
> #include <linux/module.h>
> #include <linux/regulator/consumer.h>
> #include <linux/spi/spi.h>
> -
> -#define AD7949_MASK_CHANNEL_SEL GENMASK(9, 7)
> -#define AD7949_MASK_TOTAL GENMASK(13, 0)
> -#define AD7949_OFFSET_CHANNEL_SEL 7
> -#define AD7949_CFG_READ_BACK 0x1
> -#define AD7949_CFG_REG_SIZE_BITS 14
> +#include <linux/of.h>
> +#include <linux/bitfield.h>
> +
> +#define AD7949_CFG_REG_SIZE_BITS 14
> +#define AD7949_CFG_MASK_TOTAL GENMASK(13, 0)
> +#define AD7949_CFG_APPLY BIT(13)
> +#define AD7949_CFG_CHAN_CFG GENMASK(12, 10)
> +#define AD7949_CFG_CHAN_CFG_UNIPOLAR_GND 0x7
> +#define AD7949_CFG_CHAN_SEL GENMASK(9, 7)
> +#define AD7949_CFG_BW BIT(6)
> +#define AD7949_CFG_BW_FULL 1
> +#define AD7949_CFG_REF_SEL GENMASK(5, 3)
> +#define AD7949_CFG_SEQ GENMASK(2, 1)
> +#define AD7949_CFG_SEQ_DISABLED 0x0
> +#define AD7949_CFG_READBACK BIT(0)
> +#define AD7949_CFG_READBACK_EN 0
> +#define AD7949_CFG_READBACK_DIS 1
>
> enum {
> ID_AD7949 = 0,
> @@ -24,6 +35,18 @@ enum {
> ID_AD7689,
> };
>
> +enum ad7949_ref_sel {
> + AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
> + AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
> + AD7949_REF_EXT_TEMP, /* REF + temp sensor */
> + AD7949_REF_EXT_TEMP_BUF, /* REFIN + temp sensor */
> + AD7949_REF_RSRV_4,
> + AD7949_REF_RSRV_5,
> + AD7949_REF_EXT, /* REF, no temp */
> + AD7949_REF_EXT_BUF, /* REFIN, no temp */
> + AD7949_REF_MAX,
> +};
> +
> struct ad7949_adc_spec {
> u8 num_channels;
> u8 resolution;
> @@ -41,6 +64,7 @@ struct ad7949_adc_spec {
> * @vref: regulator generating Vref
> * @iio_dev: reference to iio structure
> * @spi: reference to spi structure
> + * @ref_sel: selected reference voltage source
> * @resolution: resolution of the chip
> * @cfg: copy of the configuration register
> * @current_channel: current channel in use
> @@ -51,6 +75,7 @@ struct ad7949_adc_chip {
> struct regulator *vref;
> struct iio_dev *indio_dev;
> struct spi_device *spi;
> + enum ad7949_ref_sel ref_sel;
> u8 resolution;
> u16 cfg;
> unsigned int current_channel;
> @@ -59,7 +84,7 @@ struct ad7949_adc_chip {
>
> static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> {
> - if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> + if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
> return true;
>
> return false;
> @@ -91,7 +116,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> };
>
> ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> - ad7949_adc->buffer = ad7949_adc->cfg << shift;
> + ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> spi_message_init_with_transfers(&msg, tx, 1);
> ret = spi_sync(ad7949_adc->spi, &msg);
>
> @@ -136,8 +161,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> };
>
> ret = ad7949_spi_write_cfg(ad7949_adc,
> - channel << AD7949_OFFSET_CHANNEL_SEL,
> - AD7949_MASK_CHANNEL_SEL);
> + FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
> + AD7949_CFG_CHAN_SEL);
> if (ret)
> return ret;
>
> @@ -204,11 +229,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> - ret = regulator_get_voltage(ad7949_adc->vref);
> - if (ret < 0)
> - return ret;
> + if (ad7949_adc->vref) {
> + ret = regulator_get_voltage(ad7949_adc->vref);
> + if (ret < 0)
> + return ret;
> +
> + *val = ret / 5000;
> + } else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
> + *val = 2500;
> + } else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
> + *val = 4096;
> + } else {
> + return -EINVAL;
> + }
>
> - *val = ret / 5000;
> return IIO_VAL_INT;
> }
>
> @@ -226,7 +260,8 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
> *readval = ad7949_adc->cfg;
> else
> ret = ad7949_spi_write_cfg(ad7949_adc,
> - writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
> + writeval & AD7949_CFG_MASK_TOTAL,
> + AD7949_CFG_MASK_TOTAL);
>
> return ret;
> }
> @@ -240,10 +275,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> {
> int ret;
> int val;
> + u16 adc_config = 0;
>
> - /* Sequencer disabled, CFG readback disabled, IN0 as default channel */
> ad7949_adc->current_channel = 0;
> - ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> + ad7949_adc->cfg = 0;
> +
> + adc_config |= FIELD_PREP(AD7949_CFG_APPLY, 1);
> + adc_config |= FIELD_PREP(AD7949_CFG_CHAN_CFG,
> + AD7949_CFG_CHAN_CFG_UNIPOLAR_GND);
> + adc_config |= FIELD_PREP(AD7949_CFG_CHAN_SEL,
> + ad7949_adc->current_channel);
> + adc_config |= FIELD_PREP(AD7949_CFG_BW, AD7949_CFG_BW_FULL);
> + adc_config |= FIELD_PREP(AD7949_CFG_REF_SEL, ad7949_adc->ref_sel);
> + adc_config |= FIELD_PREP(AD7949_CFG_SEQ, AD7949_CFG_SEQ_DISABLED);
> + adc_config |= FIELD_PREP(AD7949_CFG_READBACK, AD7949_CFG_READBACK_DIS);
> +
> + ret = ad7949_spi_write_cfg(ad7949_adc,
> + adc_config,
> + AD7949_CFG_MASK_TOTAL);
>
> /*
> * Do a dummy conversion to apply the first configuration setting.
> @@ -261,6 +310,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> struct ad7949_adc_chip *ad7949_adc;
> struct iio_dev *indio_dev;
> int ret;
> + u32 temp;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
> if (!indio_dev) {
> @@ -279,21 +329,53 @@ static int ad7949_spi_probe(struct spi_device *spi)
> ad7949_adc = iio_priv(indio_dev);
> ad7949_adc->indio_dev = indio_dev;
> ad7949_adc->spi = spi;
> + ad7949_adc->vref = NULL;
>
> spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> indio_dev->num_channels = spec->num_channels;
> ad7949_adc->resolution = spec->resolution;
>
> - ad7949_adc->vref = devm_regulator_get(dev, "vref");
> - if (IS_ERR(ad7949_adc->vref)) {
> - dev_err(dev, "fail to request regulator\n");
> - return PTR_ERR(ad7949_adc->vref);
> + ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> + "adi,reference-select",
> + &temp);
> + if (ret == 0) {
> + switch (temp) {
> + case 0:
> + ad7949_adc->ref_sel = AD7949_REF_2V5;
> + break;
> + case 1:
> + ad7949_adc->ref_sel = AD7949_REF_4V0;
> + break;
> + case 2:
> + ad7949_adc->ref_sel = AD7949_REF_EXT;
> + break;
> + case 3:
> + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> + break;
> + default:
> + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> + dev_warn(dev,
> + "unknown reference-select value, using REFIN external Vref (3) by default\n");
> + }
> + } else {
> + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> + dev_warn(dev, "using external Vref by default\n");
> }
>
> - ret = regulator_enable(ad7949_adc->vref);
> - if (ret < 0) {
> - dev_err(dev, "fail to enable regulator\n");
> - return ret;
> + /* Check whether using external Vref */
> + if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
> + (ad7949_adc->ref_sel != AD7949_REF_4V0)) {
That is 'interesting' alignment...
> + ad7949_adc->vref = devm_regulator_get(dev, "vref");
> + if (IS_ERR(ad7949_adc->vref)) {
> + dev_err(dev, "fail to request regulator\n");
> + return PTR_ERR(ad7949_adc->vref);
> + }
> +
> + ret = regulator_enable(ad7949_adc->vref);
> + if (ret < 0) {
> + dev_err(dev, "fail to enable regulator\n");
> + return ret;
> + }
> }
>
> mutex_init(&ad7949_adc->lock);
> @@ -314,7 +396,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
>
> err:
> mutex_destroy(&ad7949_adc->lock);
> - regulator_disable(ad7949_adc->vref);
> + if (ad7949_adc->vref)
> + regulator_disable(ad7949_adc->vref);
>
> return ret;
> }
> @@ -326,7 +409,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
>
> iio_device_unregister(indio_dev);
> mutex_destroy(&ad7949_adc->lock);
> - regulator_disable(ad7949_adc->vref);
> + if (ad7949_adc->vref)
> + regulator_disable(ad7949_adc->vref);
>
> return 0;
> }
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/5] iio: ad7949: Support internal Vref
2019-05-13 14:53 ` [PATCH v3 2/5] iio: ad7949: Support internal Vref Adam Michaelis
2019-05-18 9:07 ` Jonathan Cameron
@ 2019-05-23 12:12 ` Alexandru Ardelean
2019-05-23 12:51 ` Ardelean, Alexandru
1 sibling, 1 reply; 16+ messages in thread
From: Alexandru Ardelean @ 2019-05-23 12:12 UTC (permalink / raw)
To: Adam Michaelis
Cc: linux-iio, Lars-Peter Clausen, Hennerich, Michael,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, charles-antoine.couret, devicetree,
brandon.maier, clayton.shotwell, Alexandru Ardelean, Stefan Popa
On Mon, May 13, 2019 at 7:16 PM Adam Michaelis
<adam.michaelis@rockwellcollins.com> wrote:
>
> Adding configurable (via device tree) options to select one of the two
> external reference voltages (REFIN as default, original implementation)
> or one of the two internal reference voltages provided by the AD7949
> part family.
>
I would run a ./scripts/checkpatch.pl on this patch (maybe also on the series).
I would only complain about style-stuff (on this patch), but those
would also get reported by checkpatch.
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> ---
> V2:
> - Add some defines to reduce use of magic numbers.
> V3:
> - Add bitfield.h macros throughout.
> - Re-think usage of device tree parameter to focus on the
> actual reference sources instead of the raw hardware
> configuration.
> ---
> drivers/iio/adc/ad7949.c | 138 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 111 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index c7fe27aa2519..b648b1ab9559 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -11,12 +11,23 @@
> #include <linux/module.h>
> #include <linux/regulator/consumer.h>
> #include <linux/spi/spi.h>
> -
> -#define AD7949_MASK_CHANNEL_SEL GENMASK(9, 7)
> -#define AD7949_MASK_TOTAL GENMASK(13, 0)
> -#define AD7949_OFFSET_CHANNEL_SEL 7
> -#define AD7949_CFG_READ_BACK 0x1
> -#define AD7949_CFG_REG_SIZE_BITS 14
> +#include <linux/of.h>
> +#include <linux/bitfield.h>
> +
> +#define AD7949_CFG_REG_SIZE_BITS 14
> +#define AD7949_CFG_MASK_TOTAL GENMASK(13, 0)
> +#define AD7949_CFG_APPLY BIT(13)
> +#define AD7949_CFG_CHAN_CFG GENMASK(12, 10)
> +#define AD7949_CFG_CHAN_CFG_UNIPOLAR_GND 0x7
> +#define AD7949_CFG_CHAN_SEL GENMASK(9, 7)
> +#define AD7949_CFG_BW BIT(6)
> +#define AD7949_CFG_BW_FULL 1
> +#define AD7949_CFG_REF_SEL GENMASK(5, 3)
> +#define AD7949_CFG_SEQ GENMASK(2, 1)
> +#define AD7949_CFG_SEQ_DISABLED 0x0
> +#define AD7949_CFG_READBACK BIT(0)
> +#define AD7949_CFG_READBACK_EN 0
> +#define AD7949_CFG_READBACK_DIS 1
>
> enum {
> ID_AD7949 = 0,
> @@ -24,6 +35,18 @@ enum {
> ID_AD7689,
> };
>
> +enum ad7949_ref_sel {
> + AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
> + AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
> + AD7949_REF_EXT_TEMP, /* REF + temp sensor */
> + AD7949_REF_EXT_TEMP_BUF, /* REFIN + temp sensor */
> + AD7949_REF_RSRV_4,
> + AD7949_REF_RSRV_5,
> + AD7949_REF_EXT, /* REF, no temp */
> + AD7949_REF_EXT_BUF, /* REFIN, no temp */
> + AD7949_REF_MAX,
> +};
> +
> struct ad7949_adc_spec {
> u8 num_channels;
> u8 resolution;
> @@ -41,6 +64,7 @@ struct ad7949_adc_spec {
> * @vref: regulator generating Vref
> * @iio_dev: reference to iio structure
> * @spi: reference to spi structure
> + * @ref_sel: selected reference voltage source
> * @resolution: resolution of the chip
> * @cfg: copy of the configuration register
> * @current_channel: current channel in use
> @@ -51,6 +75,7 @@ struct ad7949_adc_chip {
> struct regulator *vref;
> struct iio_dev *indio_dev;
> struct spi_device *spi;
> + enum ad7949_ref_sel ref_sel;
> u8 resolution;
> u16 cfg;
> unsigned int current_channel;
> @@ -59,7 +84,7 @@ struct ad7949_adc_chip {
>
> static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> {
> - if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> + if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
> return true;
>
> return false;
> @@ -91,7 +116,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> };
>
> ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> - ad7949_adc->buffer = ad7949_adc->cfg << shift;
> + ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> spi_message_init_with_transfers(&msg, tx, 1);
> ret = spi_sync(ad7949_adc->spi, &msg);
>
> @@ -136,8 +161,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> };
>
> ret = ad7949_spi_write_cfg(ad7949_adc,
> - channel << AD7949_OFFSET_CHANNEL_SEL,
> - AD7949_MASK_CHANNEL_SEL);
> + FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
> + AD7949_CFG_CHAN_SEL);
> if (ret)
> return ret;
>
> @@ -204,11 +229,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
> return IIO_VAL_INT;
>
> case IIO_CHAN_INFO_SCALE:
> - ret = regulator_get_voltage(ad7949_adc->vref);
> - if (ret < 0)
> - return ret;
> + if (ad7949_adc->vref) {
> + ret = regulator_get_voltage(ad7949_adc->vref);
> + if (ret < 0)
> + return ret;
> +
> + *val = ret / 5000;
> + } else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
> + *val = 2500;
> + } else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
> + *val = 4096;
> + } else {
> + return -EINVAL;
> + }
>
> - *val = ret / 5000;
> return IIO_VAL_INT;
> }
>
> @@ -226,7 +260,8 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
> *readval = ad7949_adc->cfg;
> else
> ret = ad7949_spi_write_cfg(ad7949_adc,
> - writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
> + writeval & AD7949_CFG_MASK_TOTAL,
> + AD7949_CFG_MASK_TOTAL);
>
> return ret;
> }
> @@ -240,10 +275,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> {
> int ret;
> int val;
> + u16 adc_config = 0;
>
> - /* Sequencer disabled, CFG readback disabled, IN0 as default channel */
> ad7949_adc->current_channel = 0;
> - ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> + ad7949_adc->cfg = 0;
> +
> + adc_config |= FIELD_PREP(AD7949_CFG_APPLY, 1);
> + adc_config |= FIELD_PREP(AD7949_CFG_CHAN_CFG,
> + AD7949_CFG_CHAN_CFG_UNIPOLAR_GND);
> + adc_config |= FIELD_PREP(AD7949_CFG_CHAN_SEL,
> + ad7949_adc->current_channel);
> + adc_config |= FIELD_PREP(AD7949_CFG_BW, AD7949_CFG_BW_FULL);
> + adc_config |= FIELD_PREP(AD7949_CFG_REF_SEL, ad7949_adc->ref_sel);
> + adc_config |= FIELD_PREP(AD7949_CFG_SEQ, AD7949_CFG_SEQ_DISABLED);
> + adc_config |= FIELD_PREP(AD7949_CFG_READBACK, AD7949_CFG_READBACK_DIS);
> +
> + ret = ad7949_spi_write_cfg(ad7949_adc,
> + adc_config,
> + AD7949_CFG_MASK_TOTAL);
>
> /*
> * Do a dummy conversion to apply the first configuration setting.
> @@ -261,6 +310,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> struct ad7949_adc_chip *ad7949_adc;
> struct iio_dev *indio_dev;
> int ret;
> + u32 temp;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
> if (!indio_dev) {
> @@ -279,21 +329,53 @@ static int ad7949_spi_probe(struct spi_device *spi)
> ad7949_adc = iio_priv(indio_dev);
> ad7949_adc->indio_dev = indio_dev;
> ad7949_adc->spi = spi;
> + ad7949_adc->vref = NULL;
>
> spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> indio_dev->num_channels = spec->num_channels;
> ad7949_adc->resolution = spec->resolution;
>
> - ad7949_adc->vref = devm_regulator_get(dev, "vref");
> - if (IS_ERR(ad7949_adc->vref)) {
> - dev_err(dev, "fail to request regulator\n");
> - return PTR_ERR(ad7949_adc->vref);
> + ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> + "adi,reference-select",
> + &temp);
> + if (ret == 0) {
> + switch (temp) {
> + case 0:
> + ad7949_adc->ref_sel = AD7949_REF_2V5;
> + break;
> + case 1:
> + ad7949_adc->ref_sel = AD7949_REF_4V0;
> + break;
> + case 2:
> + ad7949_adc->ref_sel = AD7949_REF_EXT;
> + break;
> + case 3:
> + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> + break;
> + default:
> + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> + dev_warn(dev,
> + "unknown reference-select value, using REFIN external Vref (3) by default\n");
> + }
> + } else {
> + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> + dev_warn(dev, "using external Vref by default\n");
> }
>
> - ret = regulator_enable(ad7949_adc->vref);
> - if (ret < 0) {
> - dev_err(dev, "fail to enable regulator\n");
> - return ret;
> + /* Check whether using external Vref */
> + if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
> + (ad7949_adc->ref_sel != AD7949_REF_4V0)) {
> + ad7949_adc->vref = devm_regulator_get(dev, "vref");
> + if (IS_ERR(ad7949_adc->vref)) {
> + dev_err(dev, "fail to request regulator\n");
> + return PTR_ERR(ad7949_adc->vref);
> + }
> +
> + ret = regulator_enable(ad7949_adc->vref);
> + if (ret < 0) {
> + dev_err(dev, "fail to enable regulator\n");
> + return ret;
> + }
> }
>
> mutex_init(&ad7949_adc->lock);
> @@ -314,7 +396,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
>
> err:
> mutex_destroy(&ad7949_adc->lock);
> - regulator_disable(ad7949_adc->vref);
> + if (ad7949_adc->vref)
> + regulator_disable(ad7949_adc->vref);
>
> return ret;
> }
> @@ -326,7 +409,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
>
> iio_device_unregister(indio_dev);
> mutex_destroy(&ad7949_adc->lock);
> - regulator_disable(ad7949_adc->vref);
> + if (ad7949_adc->vref)
> + regulator_disable(ad7949_adc->vref);
>
> return 0;
> }
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 2/5] iio: ad7949: Support internal Vref
2019-05-23 12:12 ` Alexandru Ardelean
@ 2019-05-23 12:51 ` Ardelean, Alexandru
0 siblings, 0 replies; 16+ messages in thread
From: Ardelean, Alexandru @ 2019-05-23 12:51 UTC (permalink / raw)
To: ardeleanalex, adam.michaelis
Cc: lars, robh+dt, knaack.h, Popa, Stefan Serban, jic23, devicetree,
charles-antoine.couret, Hennerich, Michael, linux-iio,
clayton.shotwell, mark.rutland, pmeerw, brandon.maier
On Thu, 2019-05-23 at 15:12 +0300, Alexandru Ardelean wrote:
> [External]
>
>
> On Mon, May 13, 2019 at 7:16 PM Adam Michaelis
> <adam.michaelis@rockwellcollins.com> wrote:
> >
> > Adding configurable (via device tree) options to select one of the two
> > external reference voltages (REFIN as default, original implementation)
> > or one of the two internal reference voltages provided by the AD7949
> > part family.
> >
>
So, I managed to go through the patches.
I'll propose to re-organize the patches into smaller groups.
Let's take this patch + the device-tree patch (associated with this) into another series.
Adding support for internal Vref seems pretty straightforward to me.
The SPI communication patches seem weird and require more thought/digging on our side as well.
I'll wait for Stefan to add his input as well.
Thanks
Alex
> I would run a ./scripts/checkpatch.pl on this patch (maybe also on the series).
> I would only complain about style-stuff (on this patch), but those
> would also get reported by checkpatch.
>
> > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > ---
> > V2:
> > - Add some defines to reduce use of magic numbers.
> > V3:
> > - Add bitfield.h macros throughout.
> > - Re-think usage of device tree parameter to focus on the
> > actual reference sources instead of the raw hardware
> > configuration.
> > ---
> > drivers/iio/adc/ad7949.c | 138 +++++++++++++++++++++++++++++++++++++----------
> > 1 file changed, 111 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index c7fe27aa2519..b648b1ab9559 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -11,12 +11,23 @@
> > #include <linux/module.h>
> > #include <linux/regulator/consumer.h>
> > #include <linux/spi/spi.h>
> > -
> > -#define AD7949_MASK_CHANNEL_SEL GENMASK(9, 7)
> > -#define AD7949_MASK_TOTAL GENMASK(13, 0)
> > -#define AD7949_OFFSET_CHANNEL_SEL 7
> > -#define AD7949_CFG_READ_BACK 0x1
> > -#define AD7949_CFG_REG_SIZE_BITS 14
> > +#include <linux/of.h>
> > +#include <linux/bitfield.h>
> > +
> > +#define AD7949_CFG_REG_SIZE_BITS 14
> > +#define AD7949_CFG_MASK_TOTAL GENMASK(13, 0)
> > +#define AD7949_CFG_APPLY BIT(13)
> > +#define AD7949_CFG_CHAN_CFG GENMASK(12, 10)
> > +#define AD7949_CFG_CHAN_CFG_UNIPOLAR_GND 0x7
> > +#define AD7949_CFG_CHAN_SEL GENMASK(9, 7)
> > +#define AD7949_CFG_BW BIT(6)
> > +#define AD7949_CFG_BW_FULL 1
> > +#define AD7949_CFG_REF_SEL GENMASK(5, 3)
> > +#define AD7949_CFG_SEQ GENMASK(2, 1)
> > +#define AD7949_CFG_SEQ_DISABLED 0x0
> > +#define AD7949_CFG_READBACK BIT(0)
> > +#define AD7949_CFG_READBACK_EN 0
> > +#define AD7949_CFG_READBACK_DIS 1
> >
> > enum {
> > ID_AD7949 = 0,
> > @@ -24,6 +35,18 @@ enum {
> > ID_AD7689,
> > };
> >
> > +enum ad7949_ref_sel {
> > + AD7949_REF_2V5 = 0, /* 2.5V internal ref + temp sensor */
> > + AD7949_REF_4V0, /* 4.096V internal ref + temp sensor */
> > + AD7949_REF_EXT_TEMP, /* REF + temp sensor */
> > + AD7949_REF_EXT_TEMP_BUF, /* REFIN + temp sensor */
> > + AD7949_REF_RSRV_4,
> > + AD7949_REF_RSRV_5,
> > + AD7949_REF_EXT, /* REF, no temp */
> > + AD7949_REF_EXT_BUF, /* REFIN, no temp */
> > + AD7949_REF_MAX,
> > +};
> > +
> > struct ad7949_adc_spec {
> > u8 num_channels;
> > u8 resolution;
> > @@ -41,6 +64,7 @@ struct ad7949_adc_spec {
> > * @vref: regulator generating Vref
> > * @iio_dev: reference to iio structure
> > * @spi: reference to spi structure
> > + * @ref_sel: selected reference voltage source
> > * @resolution: resolution of the chip
> > * @cfg: copy of the configuration register
> > * @current_channel: current channel in use
> > @@ -51,6 +75,7 @@ struct ad7949_adc_chip {
> > struct regulator *vref;
> > struct iio_dev *indio_dev;
> > struct spi_device *spi;
> > + enum ad7949_ref_sel ref_sel;
> > u8 resolution;
> > u16 cfg;
> > unsigned int current_channel;
> > @@ -59,7 +84,7 @@ struct ad7949_adc_chip {
> >
> > static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> > {
> > - if (!(ad7949_adc->cfg & AD7949_CFG_READ_BACK))
> > + if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
> > return true;
> >
> > return false;
> > @@ -91,7 +116,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > };
> >
> > ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> > - ad7949_adc->buffer = ad7949_adc->cfg << shift;
> > + ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> > spi_message_init_with_transfers(&msg, tx, 1);
> > ret = spi_sync(ad7949_adc->spi, &msg);
> >
> > @@ -136,8 +161,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > };
> >
> > ret = ad7949_spi_write_cfg(ad7949_adc,
> > - channel << AD7949_OFFSET_CHANNEL_SEL,
> > - AD7949_MASK_CHANNEL_SEL);
> > + FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
> > + AD7949_CFG_CHAN_SEL);
> > if (ret)
> > return ret;
> >
> > @@ -204,11 +229,20 @@ static int ad7949_spi_read_raw(struct iio_dev *indio_dev,
> > return IIO_VAL_INT;
> >
> > case IIO_CHAN_INFO_SCALE:
> > - ret = regulator_get_voltage(ad7949_adc->vref);
> > - if (ret < 0)
> > - return ret;
> > + if (ad7949_adc->vref) {
> > + ret = regulator_get_voltage(ad7949_adc->vref);
> > + if (ret < 0)
> > + return ret;
> > +
> > + *val = ret / 5000;
> > + } else if (ad7949_adc->ref_sel == AD7949_REF_2V5) {
> > + *val = 2500;
> > + } else if (ad7949_adc->ref_sel == AD7949_REF_4V0) {
> > + *val = 4096;
> > + } else {
> > + return -EINVAL;
> > + }
> >
> > - *val = ret / 5000;
> > return IIO_VAL_INT;
> > }
> >
> > @@ -226,7 +260,8 @@ static int ad7949_spi_reg_access(struct iio_dev *indio_dev,
> > *readval = ad7949_adc->cfg;
> > else
> > ret = ad7949_spi_write_cfg(ad7949_adc,
> > - writeval & AD7949_MASK_TOTAL, AD7949_MASK_TOTAL);
> > + writeval & AD7949_CFG_MASK_TOTAL,
> > + AD7949_CFG_MASK_TOTAL);
> >
> > return ret;
> > }
> > @@ -240,10 +275,24 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> > {
> > int ret;
> > int val;
> > + u16 adc_config = 0;
> >
> > - /* Sequencer disabled, CFG readback disabled, IN0 as default channel */
> > ad7949_adc->current_channel = 0;
> > - ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
> > + ad7949_adc->cfg = 0;
> > +
> > + adc_config |= FIELD_PREP(AD7949_CFG_APPLY, 1);
> > + adc_config |= FIELD_PREP(AD7949_CFG_CHAN_CFG,
> > + AD7949_CFG_CHAN_CFG_UNIPOLAR_GND);
> > + adc_config |= FIELD_PREP(AD7949_CFG_CHAN_SEL,
> > + ad7949_adc->current_channel);
> > + adc_config |= FIELD_PREP(AD7949_CFG_BW, AD7949_CFG_BW_FULL);
> > + adc_config |= FIELD_PREP(AD7949_CFG_REF_SEL, ad7949_adc->ref_sel);
> > + adc_config |= FIELD_PREP(AD7949_CFG_SEQ, AD7949_CFG_SEQ_DISABLED);
> > + adc_config |= FIELD_PREP(AD7949_CFG_READBACK, AD7949_CFG_READBACK_DIS);
> > +
> > + ret = ad7949_spi_write_cfg(ad7949_adc,
> > + adc_config,
> > + AD7949_CFG_MASK_TOTAL);
> >
> > /*
> > * Do a dummy conversion to apply the first configuration setting.
> > @@ -261,6 +310,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > struct ad7949_adc_chip *ad7949_adc;
> > struct iio_dev *indio_dev;
> > int ret;
> > + u32 temp;
> >
> > indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
> > if (!indio_dev) {
> > @@ -279,21 +329,53 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > ad7949_adc = iio_priv(indio_dev);
> > ad7949_adc->indio_dev = indio_dev;
> > ad7949_adc->spi = spi;
> > + ad7949_adc->vref = NULL;
> >
> > spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> > indio_dev->num_channels = spec->num_channels;
> > ad7949_adc->resolution = spec->resolution;
> >
> > - ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > - if (IS_ERR(ad7949_adc->vref)) {
> > - dev_err(dev, "fail to request regulator\n");
> > - return PTR_ERR(ad7949_adc->vref);
> > + ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> > + "adi,reference-select",
> > + &temp);
> > + if (ret == 0) {
> > + switch (temp) {
> > + case 0:
> > + ad7949_adc->ref_sel = AD7949_REF_2V5;
> > + break;
> > + case 1:
> > + ad7949_adc->ref_sel = AD7949_REF_4V0;
> > + break;
> > + case 2:
> > + ad7949_adc->ref_sel = AD7949_REF_EXT;
> > + break;
> > + case 3:
> > + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> > + break;
> > + default:
> > + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> > + dev_warn(dev,
> > + "unknown reference-select value, using REFIN external Vref (3) by default\n");
> > + }
> > + } else {
> > + ad7949_adc->ref_sel = AD7949_REF_EXT_BUF;
> > + dev_warn(dev, "using external Vref by default\n");
> > }
> >
> > - ret = regulator_enable(ad7949_adc->vref);
> > - if (ret < 0) {
> > - dev_err(dev, "fail to enable regulator\n");
> > - return ret;
> > + /* Check whether using external Vref */
> > + if ((ad7949_adc->ref_sel != AD7949_REF_2V5) &&
> > + (ad7949_adc->ref_sel != AD7949_REF_4V0)) {
> > + ad7949_adc->vref = devm_regulator_get(dev, "vref");
> > + if (IS_ERR(ad7949_adc->vref)) {
> > + dev_err(dev, "fail to request regulator\n");
> > + return PTR_ERR(ad7949_adc->vref);
> > + }
> > +
> > + ret = regulator_enable(ad7949_adc->vref);
> > + if (ret < 0) {
> > + dev_err(dev, "fail to enable regulator\n");
> > + return ret;
> > + }
> > }
> >
> > mutex_init(&ad7949_adc->lock);
> > @@ -314,7 +396,8 @@ static int ad7949_spi_probe(struct spi_device *spi)
> >
> > err:
> > mutex_destroy(&ad7949_adc->lock);
> > - regulator_disable(ad7949_adc->vref);
> > + if (ad7949_adc->vref)
> > + regulator_disable(ad7949_adc->vref);
> >
> > return ret;
> > }
> > @@ -326,7 +409,8 @@ static int ad7949_spi_remove(struct spi_device *spi)
> >
> > iio_device_unregister(indio_dev);
> > mutex_destroy(&ad7949_adc->lock);
> > - regulator_disable(ad7949_adc->vref);
> > + if (ad7949_adc->vref)
> > + regulator_disable(ad7949_adc->vref);
> >
> > return 0;
> > }
> > --
> > 1.9.1
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select
2019-05-13 14:53 [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Adam Michaelis
2019-05-13 14:53 ` [PATCH v3 2/5] iio: ad7949: Support internal Vref Adam Michaelis
@ 2019-05-13 14:53 ` Adam Michaelis
2019-05-14 18:23 ` Rob Herring
2019-05-13 14:53 ` [PATCH v3 4/5] iio: ad7949: Fix SPI interfacing for 14-bit messages Adam Michaelis
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Adam Michaelis @ 2019-05-13 14:53 UTC (permalink / raw)
To: linux-iio
Cc: lars, michael.hennerich, jic23, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell, Adam Michaelis
Adding optional parameter to AD7949 to specify the source for the
reference voltage signal. Default value is maintained with option '3' to
match previous version of driver.
Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
V2:
- Add some defines to reduce use of magic numbers.
V3:
- Re-think usage of device tree parameter to focus on the
actual reference sources instead of the raw hardware
configuration.
---
.../devicetree/bindings/iio/adc/ad7949.txt | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/iio/adc/ad7949.txt b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
index c7f5057356b1..e5152335c761 100644
--- a/Documentation/devicetree/bindings/iio/adc/ad7949.txt
+++ b/Documentation/devicetree/bindings/iio/adc/ad7949.txt
@@ -6,11 +6,28 @@ Required properties:
* "adi,ad7682"
* "adi,ad7689"
- reg: spi chip select number for the device
- - vref-supply: The regulator supply for ADC reference voltage
-Example:
+Optional properties:
+ - adi,reference-select: Select the reference voltage source to use
+ when converting the input voltages. Valid values are:
+ 0: Internal 2.5V reference;
+ 1: Internal 4.096V reference;
+ 2: External REF reference;
+ 3: External REFIN reference (default);
+ - vref-supply: The regulator supply for external ADC reference voltage.
+ Required if one of the external references is selected by
+ 'adi,reference-select'.
+
+Examples:
adc@0 {
compatible = "adi,ad7949";
reg = <0>;
+ adi,reference-select = <2>;
vref-supply = <&vdd_supply>;
};
+
+adc@0 {
+ compatible = "adi,ad7949";
+ reg = <0>;
+ adi,reference-select = <0>;
+};
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select
2019-05-13 14:53 ` [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select Adam Michaelis
@ 2019-05-14 18:23 ` Rob Herring
2019-05-18 9:07 ` Jonathan Cameron
0 siblings, 1 reply; 16+ messages in thread
From: Rob Herring @ 2019-05-14 18:23 UTC (permalink / raw)
To: Adam Michaelis
Cc: linux-iio, lars, michael.hennerich, jic23, knaack.h, pmeerw,
robh+dt, mark.rutland, charles-antoine.couret, devicetree,
brandon.maier, clayton.shotwell, Adam Michaelis
On Mon, 13 May 2019 09:53:03 -0500, Adam Michaelis wrote:
> Adding optional parameter to AD7949 to specify the source for the
> reference voltage signal. Default value is maintained with option '3' to
> match previous version of driver.
>
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> ---
> V2:
> - Add some defines to reduce use of magic numbers.
> V3:
> - Re-think usage of device tree parameter to focus on the
> actual reference sources instead of the raw hardware
> configuration.
> ---
> .../devicetree/bindings/iio/adc/ad7949.txt | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select
2019-05-14 18:23 ` Rob Herring
@ 2019-05-18 9:07 ` Jonathan Cameron
2019-05-23 12:16 ` Alexandru Ardelean
0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2019-05-18 9:07 UTC (permalink / raw)
To: Rob Herring
Cc: Adam Michaelis, linux-iio, lars, michael.hennerich, knaack.h,
pmeerw, robh+dt, mark.rutland, charles-antoine.couret,
devicetree, brandon.maier, clayton.shotwell
On Tue, 14 May 2019 13:23:11 -0500
Rob Herring <robh@kernel.org> wrote:
> On Mon, 13 May 2019 09:53:03 -0500, Adam Michaelis wrote:
> > Adding optional parameter to AD7949 to specify the source for the
> > reference voltage signal. Default value is maintained with option '3' to
> > match previous version of driver.
> >
> > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > ---
> > V2:
> > - Add some defines to reduce use of magic numbers.
> > V3:
> > - Re-think usage of device tree parameter to focus on the
> > actual reference sources instead of the raw hardware
> > configuration.
> > ---
> > .../devicetree/bindings/iio/adc/ad7949.txt | 21 +++++++++++++++++++--
> > 1 file changed, 19 insertions(+), 2 deletions(-)
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Looks fine to me as well. Analog review awaited before applying.
For reference
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select
2019-05-18 9:07 ` Jonathan Cameron
@ 2019-05-23 12:16 ` Alexandru Ardelean
0 siblings, 0 replies; 16+ messages in thread
From: Alexandru Ardelean @ 2019-05-23 12:16 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Rob Herring, Adam Michaelis, linux-iio, Lars-Peter Clausen,
Hennerich, Michael, Hartmut Knaack, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, charles-antoine.couret, devicetree,
brandon.maier, clayton.shotwell, Alexandru Ardelean, Stefan Popa
On Sat, May 18, 2019 at 12:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 14 May 2019 13:23:11 -0500
> Rob Herring <robh@kernel.org> wrote:
>
> > On Mon, 13 May 2019 09:53:03 -0500, Adam Michaelis wrote:
> > > Adding optional parameter to AD7949 to specify the source for the
> > > reference voltage signal. Default value is maintained with option '3' to
> > > match previous version of driver.
> > >
> > > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> > > ---
> > > V2:
> > > - Add some defines to reduce use of magic numbers.
> > > V3:
> > > - Re-think usage of device tree parameter to focus on the
> > > actual reference sources instead of the raw hardware
> > > configuration.
> > > ---
> > > .../devicetree/bindings/iio/adc/ad7949.txt | 21 +++++++++++++++++++--
> > > 1 file changed, 19 insertions(+), 2 deletions(-)
> > >
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> Looks fine to me as well. Analog review awaited before applying.
> For reference
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
CC-ing my work email
Acked-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 4/5] iio: ad7949: Fix SPI interfacing for 14-bit messages
2019-05-13 14:53 [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Adam Michaelis
2019-05-13 14:53 ` [PATCH v3 2/5] iio: ad7949: Support internal Vref Adam Michaelis
2019-05-13 14:53 ` [PATCH v3 3/5] dt-bindings: iio: ad7949: Add adi,reference-select Adam Michaelis
@ 2019-05-13 14:53 ` Adam Michaelis
2019-05-18 9:10 ` Jonathan Cameron
2019-05-13 14:53 ` [PATCH v3 5/5] iio: ad7949: Remove logic for config readback Adam Michaelis
2019-05-23 11:47 ` [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Alexandru Ardelean
4 siblings, 1 reply; 16+ messages in thread
From: Adam Michaelis @ 2019-05-13 14:53 UTC (permalink / raw)
To: linux-iio
Cc: lars, michael.hennerich, jic23, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell, Adam Michaelis
The AD7949 (but not the other two models supported by this driver) uses
samples 14 bits wide. When attempting to communicate through certain SPI
controllers that do not support word widths of 14, this fails. Adding
logic to pack the 14-bit messages into the most-significant bits of a
16-bit message or a 2-word 8-bit message for communication using more SPI
bus controllers.
Only able to test with AD7949 part on Cadence SPI, but should support
the 16-bit samples of the AD7682 and AD7689, as well.
Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
V2:
- Add some defines to reduce use of magic numbers.
V3:
- Use union for message buffer to keep messages word-aligned for
various word sizes.
- Calculate SPI bits-per-word once and use for logic throughout.
- Add logic to use SPI controller's bits-per-word field to make
the most use of the hardware's capabilities.
- Try to support SPI word widths of 16, 14, and 8 bits.
---
drivers/iio/adc/ad7949.c | 115 +++++++++++++++++++++++++++++++++++------------
1 file changed, 87 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index b648b1ab9559..d67033a008e5 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -78,10 +78,30 @@ struct ad7949_adc_chip {
enum ad7949_ref_sel ref_sel;
u8 resolution;
u16 cfg;
+ u8 bits_per_word;
unsigned int current_channel;
- u32 buffer ____cacheline_aligned;
+ union {
+ u32 buffer;
+ u16 buf16[2];
+ u8 buf8[4];
+ } ____cacheline_aligned;
};
+static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
+{
+ /* Prefer messages that match the ADC's resolution */
+ if (ad7949_adc->spi->controller->bits_per_word_mask &
+ SPI_BPW_MASK(ad7949_adc->resolution))
+ ad7949_adc->bits_per_word = ad7949_adc->resolution;
+ /* Second choice is to pad 14-bit words to 16 */
+ else if (ad7949_adc->spi->controller->bits_per_word_mask &
+ SPI_BPW_MASK(16))
+ ad7949_adc->bits_per_word = 16;
+ /* Last resort, use 8-bit words */
+ else
+ ad7949_adc->bits_per_word = 8;
+}
+
static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
{
if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
@@ -90,39 +110,63 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
return false;
}
-static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
+static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
{
- int ret = ad7949_adc->resolution;
+ int tx_len = 2;
if (ad7949_spi_cfg_is_read_back(ad7949_adc))
- ret += AD7949_CFG_REG_SIZE_BITS;
+ tx_len += 2;
- return ret;
+ return tx_len;
}
static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
u16 mask)
{
- int ret;
- int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
- int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
+ int ret = 0;
+ u16 tmp_cfg = 0;
struct spi_message msg;
struct spi_transfer tx[] = {
{
.tx_buf = &ad7949_adc->buffer,
- .len = 4,
- .bits_per_word = bits_per_word,
- },
+ .len = ad7949_message_len(ad7949_adc),
+ .bits_per_word = ad7949_adc->bits_per_word,
+ }
};
- ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
- ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
+ ad7949_adc->buffer = 0;
+
+ tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) &
+ AD7949_CFG_MASK_TOTAL;
+
+ /* If no change, return */
+ if (tmp_cfg == ad7949_adc->cfg)
+ return 0;
+
+ ad7949_adc->cfg = tmp_cfg;
+
+ switch (ad7949_adc->bits_per_word) {
+ case 16:
+ ad7949_adc->buf16[0] = ad7949_adc->cfg << 2;
+ break;
+ case 14:
+ ad7949_adc->buf16[0] = ad7949_adc->cfg;
+ break;
+ default: /* 8 */
+ /* Pack 14-bit value into 2 bytes, MSB first */
+ ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
+ ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
+ ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
+ break;
+ }
+
spi_message_init_with_transfers(&msg, tx, 1);
+
ret = spi_sync(ad7949_adc->spi, &msg);
/*
- * This delay is to avoid a new request before the required time to
- * send a new command to the device
+ * This delay is to avoid a new request before the required
+ * time to send a new command to the device
*/
udelay(2);
@@ -149,17 +193,17 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
unsigned int channel)
{
int ret;
- int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
- int mask = GENMASK(ad7949_adc->resolution, 0);
struct spi_message msg;
struct spi_transfer tx[] = {
{
.rx_buf = &ad7949_adc->buffer,
- .len = 4,
- .bits_per_word = bits_per_word,
- },
+ .len = ad7949_message_len(ad7949_adc),
+ .bits_per_word = ad7949_adc->bits_per_word,
+ }
};
+ ad7949_adc->current_channel = channel;
+
ret = ad7949_spi_write_cfg(ad7949_adc,
FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
AD7949_CFG_CHAN_SEL);
@@ -167,23 +211,37 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
return ret;
ad7949_adc->buffer = 0;
+
spi_message_init_with_transfers(&msg, tx, 1);
+
ret = spi_sync(ad7949_adc->spi, &msg);
if (ret)
return ret;
/*
- * This delay is to avoid a new request before the required time to
- * send a new command to the device
+ * This delay is to avoid a new request before the required time
+ * to send a new command to the device.
*/
udelay(2);
- ad7949_adc->current_channel = channel;
-
- if (ad7949_spi_cfg_is_read_back(ad7949_adc))
- *val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
- else
- *val = ad7949_adc->buffer & mask;
+ switch (ad7949_adc->bits_per_word) {
+ case 16:
+ *val = ad7949_adc->buf16[0];
+ /* Shift-out padding bits */
+ if (ad7949_adc->resolution == 14)
+ *val = *val >> 2;
+ break;
+ case 14:
+ *val = ad7949_adc->buf16[0] & GENMASK(13, 0);
+ break;
+ default: /* 8 */
+ /* Convert byte array to u16, MSB first */
+ *val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
+ /* Shift-out padding bits */
+ if (ad7949_adc->resolution == 14)
+ *val = *val >> 2;
+ break;
+ }
return 0;
}
@@ -334,6 +392,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
indio_dev->num_channels = spec->num_channels;
ad7949_adc->resolution = spec->resolution;
+ ad7949_set_bits_per_word(ad7949_adc);
ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
"adi,reference-select",
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/5] iio: ad7949: Fix SPI interfacing for 14-bit messages
2019-05-13 14:53 ` [PATCH v3 4/5] iio: ad7949: Fix SPI interfacing for 14-bit messages Adam Michaelis
@ 2019-05-18 9:10 ` Jonathan Cameron
2019-05-23 12:39 ` Alexandru Ardelean
0 siblings, 1 reply; 16+ messages in thread
From: Jonathan Cameron @ 2019-05-18 9:10 UTC (permalink / raw)
To: Adam Michaelis
Cc: linux-iio, lars, michael.hennerich, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell
On Mon, 13 May 2019 09:53:04 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> The AD7949 (but not the other two models supported by this driver) uses
> samples 14 bits wide. When attempting to communicate through certain SPI
> controllers that do not support word widths of 14, this fails. Adding
> logic to pack the 14-bit messages into the most-significant bits of a
> 16-bit message or a 2-word 8-bit message for communication using more SPI
> bus controllers.
>
> Only able to test with AD7949 part on Cadence SPI, but should support
> the 16-bit samples of the AD7682 and AD7689, as well.
>
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
This one looks fine to me as well. For my reference
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Note a trivial comment inline that can be tidied up whilst applying
or if you do another spin.
Jonathan
> ---
> V2:
> - Add some defines to reduce use of magic numbers.
> V3:
> - Use union for message buffer to keep messages word-aligned for
> various word sizes.
> - Calculate SPI bits-per-word once and use for logic throughout.
> - Add logic to use SPI controller's bits-per-word field to make
> the most use of the hardware's capabilities.
> - Try to support SPI word widths of 16, 14, and 8 bits.
> ---
> drivers/iio/adc/ad7949.c | 115 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 87 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index b648b1ab9559..d67033a008e5 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -78,10 +78,30 @@ struct ad7949_adc_chip {
> enum ad7949_ref_sel ref_sel;
> u8 resolution;
> u16 cfg;
> + u8 bits_per_word;
> unsigned int current_channel;
> - u32 buffer ____cacheline_aligned;
> + union {
> + u32 buffer;
> + u16 buf16[2];
> + u8 buf8[4];
> + } ____cacheline_aligned;
> };
>
> +static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> +{
> + /* Prefer messages that match the ADC's resolution */
> + if (ad7949_adc->spi->controller->bits_per_word_mask &
> + SPI_BPW_MASK(ad7949_adc->resolution))
> + ad7949_adc->bits_per_word = ad7949_adc->resolution;
> + /* Second choice is to pad 14-bit words to 16 */
> + else if (ad7949_adc->spi->controller->bits_per_word_mask &
> + SPI_BPW_MASK(16))
> + ad7949_adc->bits_per_word = 16;
> + /* Last resort, use 8-bit words */
> + else
> + ad7949_adc->bits_per_word = 8;
> +}
> +
> static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> {
> if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
> @@ -90,39 +110,63 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> return false;
> }
>
> -static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> +static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
> {
> - int ret = ad7949_adc->resolution;
> + int tx_len = 2;
>
> if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> - ret += AD7949_CFG_REG_SIZE_BITS;
> + tx_len += 2;
>
> - return ret;
> + return tx_len;
> }
>
> static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> u16 mask)
> {
> - int ret;
> - int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> - int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> + int ret = 0;
> + u16 tmp_cfg = 0;
> struct spi_message msg;
> struct spi_transfer tx[] = {
> {
> .tx_buf = &ad7949_adc->buffer,
> - .len = 4,
> - .bits_per_word = bits_per_word,
> - },
> + .len = ad7949_message_len(ad7949_adc),
> + .bits_per_word = ad7949_adc->bits_per_word,
> + }
> };
>
> - ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> - ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> + ad7949_adc->buffer = 0;
> +
> + tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) &
> + AD7949_CFG_MASK_TOTAL;
> +
> + /* If no change, return */
> + if (tmp_cfg == ad7949_adc->cfg)
> + return 0;
> +
> + ad7949_adc->cfg = tmp_cfg;
> +
> + switch (ad7949_adc->bits_per_word) {
> + case 16:
> + ad7949_adc->buf16[0] = ad7949_adc->cfg << 2;
> + break;
> + case 14:
> + ad7949_adc->buf16[0] = ad7949_adc->cfg;
> + break;
> + default: /* 8 */
> + /* Pack 14-bit value into 2 bytes, MSB first */
> + ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> + ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
> + ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
> + break;
> + }
> +
> spi_message_init_with_transfers(&msg, tx, 1);
> +
> ret = spi_sync(ad7949_adc->spi, &msg);
>
> /*
> - * This delay is to avoid a new request before the required time to
> - * send a new command to the device
> + * This delay is to avoid a new request before the required
> + * time to send a new command to the device
> */
> udelay(2);
>
> @@ -149,17 +193,17 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> unsigned int channel)
> {
> int ret;
> - int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> - int mask = GENMASK(ad7949_adc->resolution, 0);
> struct spi_message msg;
> struct spi_transfer tx[] = {
> {
> .rx_buf = &ad7949_adc->buffer,
> - .len = 4,
> - .bits_per_word = bits_per_word,
> - },
> + .len = ad7949_message_len(ad7949_adc),
> + .bits_per_word = ad7949_adc->bits_per_word,
> + }
> };
>
> + ad7949_adc->current_channel = channel;
> +
> ret = ad7949_spi_write_cfg(ad7949_adc,
> FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
> AD7949_CFG_CHAN_SEL);
> @@ -167,23 +211,37 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> return ret;
>
> ad7949_adc->buffer = 0;
> +
> spi_message_init_with_transfers(&msg, tx, 1);
> +
Unrelated change. Nothing wrong with it, but not in a patch making
functional changes.
> ret = spi_sync(ad7949_adc->spi, &msg);
> if (ret)
> return ret;
>
> /*
> - * This delay is to avoid a new request before the required time to
> - * send a new command to the device
> + * This delay is to avoid a new request before the required time
> + * to send a new command to the device.
> */
> udelay(2);
>
> - ad7949_adc->current_channel = channel;
> -
> - if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> - *val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
> - else
> - *val = ad7949_adc->buffer & mask;
> + switch (ad7949_adc->bits_per_word) {
> + case 16:
> + *val = ad7949_adc->buf16[0];
> + /* Shift-out padding bits */
> + if (ad7949_adc->resolution == 14)
> + *val = *val >> 2;
> + break;
> + case 14:
> + *val = ad7949_adc->buf16[0] & GENMASK(13, 0);
> + break;
> + default: /* 8 */
> + /* Convert byte array to u16, MSB first */
> + *val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
> + /* Shift-out padding bits */
> + if (ad7949_adc->resolution == 14)
> + *val = *val >> 2;
> + break;
> + }
>
> return 0;
> }
> @@ -334,6 +392,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> indio_dev->num_channels = spec->num_channels;
> ad7949_adc->resolution = spec->resolution;
> + ad7949_set_bits_per_word(ad7949_adc);
>
> ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> "adi,reference-select",
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 4/5] iio: ad7949: Fix SPI interfacing for 14-bit messages
2019-05-18 9:10 ` Jonathan Cameron
@ 2019-05-23 12:39 ` Alexandru Ardelean
0 siblings, 0 replies; 16+ messages in thread
From: Alexandru Ardelean @ 2019-05-23 12:39 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Adam Michaelis, linux-iio, Lars-Peter Clausen, Hennerich,
Michael, Hartmut Knaack, Peter Meerwald-Stadler, Rob Herring,
Mark Rutland, charles-antoine.couret, devicetree, Brandon Maier,
Clayton Shotwell, Alexandru Ardelean, Stefan Popa
On Sat, May 18, 2019 at 12:11 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 13 May 2019 09:53:04 -0500
> Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
>
CC-ing my work email.
> > The AD7949 (but not the other two models supported by this driver) uses
> > samples 14 bits wide. When attempting to communicate through certain SPI
> > controllers that do not support word widths of 14, this fails. Adding
> > logic to pack the 14-bit messages into the most-significant bits of a
> > 16-bit message or a 2-word 8-bit message for communication using more SPI
> > bus controllers.
> >
General note, there are some changes in this patch that are
unrelated/un-needed to the semantic of the patch.
I'm seeing some comments being re-arranged, and some newlines being added.
Jonathan also mentioned something about them.
It's good practice to not touch whitespace, or style in a patch that
is mostly functional.
For now, this is fine as-is from my side as well.
In any case, sticking to this patch:
I am wondering whether this driver needs to care about SPI packing
logic, or the SPI controller needs to do that, or the SPI framework ?
I would assume, that the way to do things, is to provide the SPI
framework the parameters of the data (shifting, bits-per-word, etc)
and the SPI framework would do the rest.
> > Only able to test with AD7949 part on Cadence SPI, but should support
> > the 16-bit samples of the AD7682 and AD7689, as well.
> >
> > Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> This one looks fine to me as well. For my reference
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Note a trivial comment inline that can be tidied up whilst applying
> or if you do another spin.
>
> Jonathan
> > ---
> > V2:
> > - Add some defines to reduce use of magic numbers.
> > V3:
> > - Use union for message buffer to keep messages word-aligned for
> > various word sizes.
> > - Calculate SPI bits-per-word once and use for logic throughout.
> > - Add logic to use SPI controller's bits-per-word field to make
> > the most use of the hardware's capabilities.
> > - Try to support SPI word widths of 16, 14, and 8 bits.
> > ---
> > drivers/iio/adc/ad7949.c | 115 +++++++++++++++++++++++++++++++++++------------
> > 1 file changed, 87 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > index b648b1ab9559..d67033a008e5 100644
> > --- a/drivers/iio/adc/ad7949.c
> > +++ b/drivers/iio/adc/ad7949.c
> > @@ -78,10 +78,30 @@ struct ad7949_adc_chip {
> > enum ad7949_ref_sel ref_sel;
> > u8 resolution;
> > u16 cfg;
> > + u8 bits_per_word;
> > unsigned int current_channel;
> > - u32 buffer ____cacheline_aligned;
> > + union {
> > + u32 buffer;
> > + u16 buf16[2];
> > + u8 buf8[4];
> > + } ____cacheline_aligned;
> > };
> >
> > +static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> > +{
> > + /* Prefer messages that match the ADC's resolution */
> > + if (ad7949_adc->spi->controller->bits_per_word_mask &
> > + SPI_BPW_MASK(ad7949_adc->resolution))
My sentiment is that this mail evaluate to true even when you don't want it to.
I'm guessing the intention here was something like:
u32 resolution_msk = SPI_BPW_MASK(ad7949_adc->resolution);
u32 resolution = resolution_msk;
resolution &= ad7949_adc->spi->controller->bits_per_word_mask;
if (resolution == resolution_msk)
ad7949_adc->bits_per_word = ad7949_adc->resolution;
else if (resolution == SPI_BPW_MASK(16))
ad7949_adc->bits_per_word = 16;
else
ad7949_adc->bits_per_word = 8;
> > + ad7949_adc->bits_per_word = ad7949_adc->resolution;
> > + /* Second choice is to pad 14-bit words to 16 */
> > + else if (ad7949_adc->spi->controller->bits_per_word_mask &
> > + SPI_BPW_MASK(16))
> > + ad7949_adc->bits_per_word = 16;
> > + /* Last resort, use 8-bit words */
> > + else
> > + ad7949_adc->bits_per_word = 8;
> > +}
> > +
> > static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> > {
> > if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
> > @@ -90,39 +110,63 @@ static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> > return false;
> > }
> >
> > -static int ad7949_spi_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> > +static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
> > {
> > - int ret = ad7949_adc->resolution;
> > + int tx_len = 2;
> >
> > if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> > - ret += AD7949_CFG_REG_SIZE_BITS;
> > + tx_len += 2;
> >
> > - return ret;
> > + return tx_len;
> > }
> >
> > static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> > u16 mask)
> > {
> > - int ret;
> > - int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> > - int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
> > + int ret = 0;
> > + u16 tmp_cfg = 0;
> > struct spi_message msg;
> > struct spi_transfer tx[] = {
> > {
> > .tx_buf = &ad7949_adc->buffer,
> > - .len = 4,
> > - .bits_per_word = bits_per_word,
> > - },
> > + .len = ad7949_message_len(ad7949_adc),
> > + .bits_per_word = ad7949_adc->bits_per_word,
> > + }
> > };
> >
> > - ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask);
> > - ad7949_adc->buffer = (ad7949_adc->cfg & AD7949_CFG_MASK_TOTAL) << shift;
> > + ad7949_adc->buffer = 0;
> > +
> > + tmp_cfg = ((val & mask) | (ad7949_adc->cfg & ~mask)) &
> > + AD7949_CFG_MASK_TOTAL;
> > +
> > + /* If no change, return */
> > + if (tmp_cfg == ad7949_adc->cfg)
> > + return 0;
> > +
> > + ad7949_adc->cfg = tmp_cfg;
> > +
> > + switch (ad7949_adc->bits_per_word) {
> > + case 16:
> > + ad7949_adc->buf16[0] = ad7949_adc->cfg << 2;
> > + break;
> > + case 14:
> > + ad7949_adc->buf16[0] = ad7949_adc->cfg;
> > + break;
> > + default: /* 8 */
> > + /* Pack 14-bit value into 2 bytes, MSB first */
> > + ad7949_adc->buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
> > + ad7949_adc->buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg);
> > + ad7949_adc->buf8[1] = ad7949_adc->buf8[1] << 2;
> > + break;
> > + }
> > +
> > spi_message_init_with_transfers(&msg, tx, 1);
> > +
> > ret = spi_sync(ad7949_adc->spi, &msg);
> >
> > /*
> > - * This delay is to avoid a new request before the required time to
> > - * send a new command to the device
> > + * This delay is to avoid a new request before the required
> > + * time to send a new command to the device
> > */
> > udelay(2);
> >
> > @@ -149,17 +193,17 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > unsigned int channel)
> > {
> > int ret;
> > - int bits_per_word = ad7949_spi_bits_per_word(ad7949_adc);
> > - int mask = GENMASK(ad7949_adc->resolution, 0);
> > struct spi_message msg;
> > struct spi_transfer tx[] = {
> > {
> > .rx_buf = &ad7949_adc->buffer,
> > - .len = 4,
> > - .bits_per_word = bits_per_word,
> > - },
> > + .len = ad7949_message_len(ad7949_adc),
> > + .bits_per_word = ad7949_adc->bits_per_word,
> > + }
> > };
> >
> > + ad7949_adc->current_channel = channel;
> > +
> > ret = ad7949_spi_write_cfg(ad7949_adc,
> > FIELD_PREP(AD7949_CFG_CHAN_SEL, channel),
> > AD7949_CFG_CHAN_SEL);
> > @@ -167,23 +211,37 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> > return ret;
> >
> > ad7949_adc->buffer = 0;
> > +
> > spi_message_init_with_transfers(&msg, tx, 1);
> > +
> Unrelated change. Nothing wrong with it, but not in a patch making
> functional changes.
>
> > ret = spi_sync(ad7949_adc->spi, &msg);
> > if (ret)
> > return ret;
> >
> > /*
> > - * This delay is to avoid a new request before the required time to
> > - * send a new command to the device
> > + * This delay is to avoid a new request before the required time
> > + * to send a new command to the device.
> > */
> > udelay(2);
> >
> > - ad7949_adc->current_channel = channel;
> > -
> > - if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> > - *val = (ad7949_adc->buffer >> AD7949_CFG_REG_SIZE_BITS) & mask;
> > - else
> > - *val = ad7949_adc->buffer & mask;
> > + switch (ad7949_adc->bits_per_word) {
> > + case 16:
> > + *val = ad7949_adc->buf16[0];
> > + /* Shift-out padding bits */
> > + if (ad7949_adc->resolution == 14)
> > + *val = *val >> 2;
> > + break;
> > + case 14:
> > + *val = ad7949_adc->buf16[0] & GENMASK(13, 0);
> > + break;
> > + default: /* 8 */
> > + /* Convert byte array to u16, MSB first */
> > + *val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
> > + /* Shift-out padding bits */
> > + if (ad7949_adc->resolution == 14)
> > + *val = *val >> 2;
> > + break;
> > + }
> >
> > return 0;
> > }
> > @@ -334,6 +392,7 @@ static int ad7949_spi_probe(struct spi_device *spi)
> > spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
> > indio_dev->num_channels = spec->num_channels;
> > ad7949_adc->resolution = spec->resolution;
> > + ad7949_set_bits_per_word(ad7949_adc);
> >
> > ret = of_property_read_u32(ad7949_adc->indio_dev->dev.of_node,
> > "adi,reference-select",
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 5/5] iio: ad7949: Remove logic for config readback
2019-05-13 14:53 [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Adam Michaelis
` (2 preceding siblings ...)
2019-05-13 14:53 ` [PATCH v3 4/5] iio: ad7949: Fix SPI interfacing for 14-bit messages Adam Michaelis
@ 2019-05-13 14:53 ` Adam Michaelis
2019-05-18 9:19 ` Jonathan Cameron
2019-05-23 11:47 ` [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Alexandru Ardelean
4 siblings, 1 reply; 16+ messages in thread
From: Adam Michaelis @ 2019-05-13 14:53 UTC (permalink / raw)
To: linux-iio
Cc: lars, michael.hennerich, jic23, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell, Adam Michaelis
The AD7949 has a feature to include the configuration register value
used to generate the ADC sample. This feature is configurable, but do
not see a good use case for it in the driver (neither did reviewing
maintainers), so removing the supporting logic in order to simplify the
driver.
Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
---
V3:
- First version of series with this patch. Maintainers agreed
that this capability of the hardware is not useful and should
be dropped from driver.
---
drivers/iio/adc/ad7949.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index d67033a008e5..bac16a2f7850 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -102,24 +102,6 @@ static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
ad7949_adc->bits_per_word = 8;
}
-static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
-{
- if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
- return true;
-
- return false;
-}
-
-static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
-{
- int tx_len = 2;
-
- if (ad7949_spi_cfg_is_read_back(ad7949_adc))
- tx_len += 2;
-
- return tx_len;
-}
-
static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
u16 mask)
{
@@ -129,7 +111,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
struct spi_transfer tx[] = {
{
.tx_buf = &ad7949_adc->buffer,
- .len = ad7949_message_len(ad7949_adc),
+ .len = 2,
.bits_per_word = ad7949_adc->bits_per_word,
}
};
@@ -197,7 +179,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
struct spi_transfer tx[] = {
{
.rx_buf = &ad7949_adc->buffer,
- .len = ad7949_message_len(ad7949_adc),
+ .len = 2,
.bits_per_word = ad7949_adc->bits_per_word,
}
};
--
1.9.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v3 5/5] iio: ad7949: Remove logic for config readback
2019-05-13 14:53 ` [PATCH v3 5/5] iio: ad7949: Remove logic for config readback Adam Michaelis
@ 2019-05-18 9:19 ` Jonathan Cameron
0 siblings, 0 replies; 16+ messages in thread
From: Jonathan Cameron @ 2019-05-18 9:19 UTC (permalink / raw)
To: Adam Michaelis
Cc: linux-iio, lars, michael.hennerich, knaack.h, pmeerw, robh+dt,
mark.rutland, charles-antoine.couret, devicetree, brandon.maier,
clayton.shotwell
On Mon, 13 May 2019 09:53:05 -0500
Adam Michaelis <adam.michaelis@rockwellcollins.com> wrote:
> The AD7949 has a feature to include the configuration register value
> used to generate the ADC sample. This feature is configurable, but do
> not see a good use case for it in the driver (neither did reviewing
> maintainers), so removing the supporting logic in order to simplify the
> driver.
>
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
Excellent. For reference
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Waiting for Analog review on the whole series.
Thanks,
Jonathan
> ---
> V3:
> - First version of series with this patch. Maintainers agreed
> that this capability of the hardware is not useful and should
> be dropped from driver.
> ---
> drivers/iio/adc/ad7949.c | 22 ++--------------------
> 1 file changed, 2 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index d67033a008e5..bac16a2f7850 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -102,24 +102,6 @@ static void ad7949_set_bits_per_word(struct ad7949_adc_chip *ad7949_adc)
> ad7949_adc->bits_per_word = 8;
> }
>
> -static bool ad7949_spi_cfg_is_read_back(struct ad7949_adc_chip *ad7949_adc)
> -{
> - if (!(ad7949_adc->cfg & AD7949_CFG_READBACK))
> - return true;
> -
> - return false;
> -}
> -
> -static int ad7949_message_len(struct ad7949_adc_chip *ad7949_adc)
> -{
> - int tx_len = 2;
> -
> - if (ad7949_spi_cfg_is_read_back(ad7949_adc))
> - tx_len += 2;
> -
> - return tx_len;
> -}
> -
> static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> u16 mask)
> {
> @@ -129,7 +111,7 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> struct spi_transfer tx[] = {
> {
> .tx_buf = &ad7949_adc->buffer,
> - .len = ad7949_message_len(ad7949_adc),
> + .len = 2,
> .bits_per_word = ad7949_adc->bits_per_word,
> }
> };
> @@ -197,7 +179,7 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
> struct spi_transfer tx[] = {
> {
> .rx_buf = &ad7949_adc->buffer,
> - .len = ad7949_message_len(ad7949_adc),
> + .len = 2,
> .bits_per_word = ad7949_adc->bits_per_word,
> }
> };
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement
2019-05-13 14:53 [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Adam Michaelis
` (3 preceding siblings ...)
2019-05-13 14:53 ` [PATCH v3 5/5] iio: ad7949: Remove logic for config readback Adam Michaelis
@ 2019-05-23 11:47 ` Alexandru Ardelean
2019-05-24 11:49 ` Couret Charles-Antoine
4 siblings, 1 reply; 16+ messages in thread
From: Alexandru Ardelean @ 2019-05-23 11:47 UTC (permalink / raw)
To: Adam Michaelis
Cc: linux-iio, Lars-Peter Clausen, Hennerich, Michael,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, charles-antoine.couret, devicetree,
brandon.maier, clayton.shotwell, Alexandru Ardelean, Stefan Popa
On Mon, May 13, 2019 at 7:18 PM Adam Michaelis
<adam.michaelis@rockwellcollins.com> wrote:
>
> The AD7949 requires two conversion cycles following the first
> configuration change, and one extra cycle following any other
> configuration change (including changing the analog channel being
> sampled). Therefore, adding a dummy read cycle when config is changed
> and removing the extra cycle at initial configuration (the first dummy
> cycle is now performed as part of applying the configuration change).
>
CC-ing my work email.
And Stefan as well.
We'll have to try this driver & changes internally a bit.
Hopefully we have a compatible device around the office.
Since it wasn't written by us, we're also unsure about things just by
looking at the code.
> Signed-off-by: Adam Michaelis <adam.michaelis@rockwellcollins.com>
> ---
> V2:
> - Add some defines to reduce use of magic numbers.
> V3:
> - Switch back to using a u32 data buffer.
> - Add-back the second dummy cycle on initialization.
> - Move to first patch in series.
> ---
> drivers/iio/adc/ad7949.c | 20 ++++++++++++++++++--
> 1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index ac0ffff6c5ae..c7fe27aa2519 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -100,6 +100,23 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
> * send a new command to the device
> */
> udelay(2);
> +
> + /*
> + * Perform extra read cycle to allow configuration, acquisition,
> + * and conversion sequences to complete for new configuration.
> + */
> + ad7949_adc->buffer = 0;
> +
> + spi_message_init_with_transfers(&msg, tx, 1);
> +
> + ret = spi_sync(ad7949_adc->spi, &msg);
> +
> + /*
> + * This delay is to avoid a new request before the required time
> + * to send a new command to the device.
> + */
> + udelay(2);
> +
Is this needed if the channel doesn't change ?
If it isn't, maybe add a check in ad7949_spi_read_channel() to skip
the call to ad7949_spi_write_cfg().
We should also take performance into account when doing SPI
transactions to the device, and if we can skip some of them, all the
better.
This change, introduces a performance penalty by doing this extra read + udelay.
> return ret;
> }
>
> @@ -229,11 +246,10 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
> ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
>
> /*
> - * Do two dummy conversions to apply the first configuration setting.
> + * Do a dummy conversion to apply the first configuration setting.
> * Required only after the start up of the device.
> */
> ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
> - ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
The datasheet mentions that 2 dummy conversions are needed on power-up / init.
The way, this was done here was a bit easier to follow (or
straightforward) with the datasheet.
This isn't to say that this is bad, but if we need to do an extra SPI
read (and skip the SPI write part), then I would just add an SPI read
function, instead of moving it completely into ad7949_spi_write_cfg(),
which would then compact things in ad7949_spi_read_channel().
>
> return ret;
> }
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement
2019-05-23 11:47 ` [PATCH v3 1/5] iio: ad7949: Fix dummy read cycle placement Alexandru Ardelean
@ 2019-05-24 11:49 ` Couret Charles-Antoine
0 siblings, 0 replies; 16+ messages in thread
From: Couret Charles-Antoine @ 2019-05-24 11:49 UTC (permalink / raw)
To: Alexandru Ardelean, Adam Michaelis
Cc: linux-iio, Lars-Peter Clausen, Hennerich, Michael,
Jonathan Cameron, Hartmut Knaack, Peter Meerwald-Stadler,
Rob Herring, Mark Rutland, devicetree, brandon.maier,
clayton.shotwell, Alexandru Ardelean, Stefan Popa
Le 23/05/2019 à 13:47, Alexandru Ardelean a écrit :
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
>> index ac0ffff6c5ae..c7fe27aa2519 100644
>> --- a/drivers/iio/adc/ad7949.c
>> +++ b/drivers/iio/adc/ad7949.c
>> @@ -100,6 +100,23 @@ static int ad7949_spi_write_cfg(struct ad7949_adc_chip *ad7949_adc, u16 val,
>> * send a new command to the device
>> */
>> udelay(2);
>> +
>> + /*
>> + * Perform extra read cycle to allow configuration, acquisition,
>> + * and conversion sequences to complete for new configuration.
>> + */
>> + ad7949_adc->buffer = 0;
>> +
>> + spi_message_init_with_transfers(&msg, tx, 1);
>> +
>> + ret = spi_sync(ad7949_adc->spi, &msg);
>> +
>> + /*
>> + * This delay is to avoid a new request before the required time
>> + * to send a new command to the device.
>> + */
>> + udelay(2);
>> +
> Is this needed if the channel doesn't change ?
> If it isn't, maybe add a check in ad7949_spi_read_channel() to skip
> the call to ad7949_spi_write_cfg().
>
> We should also take performance into account when doing SPI
> transactions to the device, and if we can skip some of them, all the
> better.
> This change, introduces a performance penalty by doing this extra read + udelay.
I wrote the initial driver and you're right. It is not required in the
situation where the channel is not changed.
In our product the channel changed every time so we didn't think about
this kind of optimisations. It is relevant to implement them.
>> return ret;
>> }
>>
>> @@ -229,11 +246,10 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
>> ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
>>
>> /*
>> - * Do two dummy conversions to apply the first configuration setting.
>> + * Do a dummy conversion to apply the first configuration setting.
>> * Required only after the start up of the device.
>> */
>> ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
>> - ad7949_spi_read_channel(ad7949_adc, &val, ad7949_adc->current_channel);
> The datasheet mentions that 2 dummy conversions are needed on power-up / init.
>
> The way, this was done here was a bit easier to follow (or
> straightforward) with the datasheet.
>
> This isn't to say that this is bad, but if we need to do an extra SPI
> read (and skip the SPI write part), then I would just add an SPI read
> function, instead of moving it completely into ad7949_spi_write_cfg(),
> which would then compact things in ad7949_spi_read_channel().
+1. It is more readable from my point of view to have two explicit
commands for this step.
I agree with your solution.
Regards,
Charles-Antoine Couret
^ permalink raw reply [flat|nested] 16+ messages in thread