AD7949 Fixes
Subsystems (3)
cover AD7949 Fixes
While working on another series[1] I ran into issues where my SPI controller would fail to handle 14-bit and 16-bit SPI messages. This addresses that issue and adds support for selecting a different voltage reference source from the devicetree.
This is base on a series[2] that seems to not have made it all the way, and was tested on an ad7689.
[1] https://patchwork.kernel.org/project/linux-iio/list/?series=511545
[2] https://patchwork.kernel.org/project/linux-iio/list/?series=116971&state=%2A&archive=both
Changes since v1:
- add default case in read/write size cases
- drop of_node change as the core already takes care of it
- check dt user input in probe
- move description at the top of dt-binding definition
- drop AllOf block in dt-binding
Thanks for your time,
Liam
Liam Beguin (4):
iio: adc: ad7949: define and use bitfield names
iio: adc: ad7949: fix spi messages on non 14-bit controllers
iio: adc: ad7949: add support for internal vref
dt-bindings: iio: adc: ad7949: add adi,reference-source
.../bindings/iio/adc/adi,ad7949.yaml | 21 ++ drivers/iio/adc/ad7949.c | 201 +++++++++++++++--- 2 files changed, 192 insertions(+), 30 deletions(-)
Range-diff against v1:
-: ------------ > 1: b8577e93229f iio: adc: ad7949: define and use bitfield names
1: 86bab3bedcf8 ! 2: a8468391e3d0 iio: adc: ad7949: fix spi messages on non 14-bit controllers
@@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
+ ad7949_adc->buffer = ad7949_adc->cfg;
+ break;
+ case 8:
++ default:
+ /* 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);
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
+ *val = ad7949_adc->buffer & GENMASK(13, 0);
+ break;
+ case 8:
++ default:
+ /* Convert byte array to u16, MSB first */
+ *val = (ad7949_adc->buf8[0] << 8) | ad7949_adc->buf8[1];
+ /* Shift-out padding bits */
2: 5f4dbdd51e1f ! 3: 4b0c11c9a748 iio: adc: ad7949: add support for internal vref
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_init(struct ad7949_adc_chip *ad7
const struct ad7949_adc_spec *spec;
struct ad7949_adc_chip *ad7949_adc;
struct iio_dev *indio_dev;
-@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
- return -ENOMEM;
- }
-
-+ indio_dev->dev.of_node = np;
- indio_dev->info = &ad7949_spi_info;
- indio_dev->name = spi_get_device_id(spi)->name;
- indio_dev->modes = INDIO_DIRECT_MODE;
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
ad7949_adc->resolution = spec->resolution;
ad7949_set_bits_per_word(ad7949_adc);
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
- if (IS_ERR(ad7949_adc->vref)) {
- dev_err(dev, "fail to request regulator\n");
- return PTR_ERR(ad7949_adc->vref);
-- }
+ /* Set default devicetree parameters */
+ ad7949_adc->refsel = AD7949_REF_EXT_BUF;
+ of_property_read_u32(np, "adi,reference-select", &ad7949_adc->refsel);
++ switch (ad7949_adc->refsel) {
++ case AD7949_REF_INT_2500:
++ case AD7949_REF_INT_4096:
++ case AD7949_REF_EXT_TEMP:
++ case AD7949_REF_EXT_TEMP_BUF:
++ case AD7949_REF_EXT:
++ case AD7949_REF_EXT_BUF:
++ break;
++ default:
++ dev_err(dev, "invalid adi,reference-select value (%d)\n",
++ ad7949_adc->refsel);
++ return -EINVAL;
+ }
- ret = regulator_enable(ad7949_adc->vref);
- if (ret < 0) {3: 99367ba6e5f9 ! 4: a3b6a6ef15fd dt-bindings: iio: adc: ad7949: add adi,reference-source
@@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
const: 1
+ adi,reference-select:
-+ allOf:
-+ - $ref: /schemas/types.yaml#/definitions/uint32
-+ - enum: [0, 1, 2, 3, 6, 7]
-+
-+ default: 7
+ description: |
+ Select the reference voltage source to use when converting samples.
+ Acceptable values are:
@@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
+ disabled.
+ - 7: Use external reference, internal buffer enabled.
+ Internal reference and temperature sensor disabled.
++
++ $ref: /schemas/types.yaml#/definitions/uint32
++ enum: [0, 1, 2, 3, 6, 7]
++ default: 7
+
required:
- compatible
base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
Signature · 1 line
2.30.1.489.g328c10930387
PATCH v2 1/4 iio: adc: ad7949: define and use bitfield names
From: Liam Beguin <[email protected]>
Replace raw configuration register values by using FIELD_PREP and defines to improve readability.
--- drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)
drivers/iio/adc/ad7949.c +31 -7
@@ -12,12 +12,27 @@#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 +#define AD7949_CFG_BIT_CFG BIT(13) +#define AD7949_CFG_VAL_CFG_OVERWRITE 1 +#define AD7949_CFG_VAL_CFG_KEEP 0 +#define AD7949_CFG_BIT_INCC GENMASK(12, 10) +#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND 7 +#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM 6 +#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF 4 +#define AD7949_CFG_VAL_INCC_TEMP 3 +#define AD7949_CFG_VAL_INCC_BIPOLAR 2 +#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF 0 +#define AD7949_CFG_BIT_INX GENMASK(9, 7) +#define AD7949_CFG_BIT_BW BIT(6) +#define AD7949_CFG_VAL_BW_FULL 1 +#define AD7949_CFG_VAL_BW_QUARTER 0 +#define AD7949_CFG_BIT_REF GENMASK(5, 3) +#define AD7949_CFG_BIT_SEQ GENMASK(2, 1) +#define AD7949_CFG_BIT_RBN BIT(0) +enum {ID_AD7949 =0 ,ID_AD7682 ,
@@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,*/ for (i =0 ; i <2 ; i++) { ret =ad7949_spi_write_cfg (ad7949_adc, - channel <<AD7949_OFFSET_CHANNEL_SEL , -AD7949_MASK_CHANNEL_SEL ); +FIELD_PREP (AD7949_CFG_BIT_INX , channel), +AD7949_CFG_BIT_INX );if (ret)return ret;if (channel == ad7949_adc->current_channel)
@@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc) {int ret;int val; +u16 cfg; -/* 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 ); + + cfg =FIELD_PREP (AD7949_CFG_BIT_CFG ,AD7949_CFG_VAL_CFG_OVERWRITE ) | +FIELD_PREP (AD7949_CFG_BIT_INCC ,AD7949_CFG_VAL_INCC_UNIPOLAR_GND ) | +FIELD_PREP (AD7949_CFG_BIT_INX , ad7949_adc->current_channel) | +FIELD_PREP (AD7949_CFG_BIT_BW ,AD7949_CFG_VAL_BW_FULL ) | +FIELD_PREP (AD7949_CFG_BIT_REF ,AD7949_REF_EXT_BUF ) | +FIELD_PREP (AD7949_CFG_BIT_SEQ ,0x0 ) | +FIELD_PREP (AD7949_CFG_BIT_RBN ,1 ); + + ret =ad7949_spi_write_cfg (ad7949_adc, cfg,AD7949_MASK_TOTAL );/* * Do two dummy conversions to apply the first configuration setting.
Signature · 1 line
2.30.1.489.g328c10930387
Jonathan Cameron
maintainerRe: [PATCH v2 1/4] iio: adc: ad7949: define and use bitfield names
Quoted 6 lines · On Fri, 9 Jul 2021 11:58:53 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
Replace raw configuration register values by using
FIELD_PREPand defines to improve readability.Signed-off-by Liam Beguin <[email protected]>
Ideally fixes should come before any refactors / cleanups like this one. That reduces the burden if people want to backport them.
In this particular case I'm guessing no one ran into the issues the following patches deal with so we can just take these in the order you have here.
Otherwise, good cleanup. A few minor comments inline, mostly as a result of some less than ideal name choices on the datasheet.
Quoted 9 lines
--- drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)drivers/iio/adc/ad7949.c +0
-0
drivers/iio/adc/ad7949.c line 18
Quoted 10 lines
drivers/iio/adc/ad7949.c +1
-3@@ -12,12 +12,27 @@#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 +#define AD7949_CFG_BIT_CFG BIT(13)
Even though that's the name on the datasheet it is silly!
I would have just one define calledAD7949_CFG_VAL_OVERWRITE BIT(13)
It's common to do that for single flags where FIELD_PREP(AD7949_CFG_VAL_OVERWRITE, 1) for example has an obvious meaning for the 1.
drivers/iio/adc/ad7949.c line 28
Quoted 10 lines
drivers/iio/adc/ad7949.c +10
-0+#define AD7949_CFG_VAL_CFG_OVERWRITE 1 +#define AD7949_CFG_VAL_CFG_KEEP 0 +#define AD7949_CFG_BIT_INCC GENMASK(12, 10) +#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND 7 +#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM 6 +#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF 4 +#define AD7949_CFG_VAL_INCC_TEMP 3 +#define AD7949_CFG_VAL_INCC_BIPOLAR 2 +#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF 0 +#define AD7949_CFG_BIT_INX GENMASK(9, 7)
This is rather non obvious abbreviation. _INx would be clearer perhaps, but then we'd get someone fixing the camel case... Given it would be good to match the datasheet, keep the name but add a comment to say this is the input channel select.
drivers/iio/adc/ad7949.c line 29
Quoted 1 line
drivers/iio/adc/ad7949.c +1
-0+#define AD7949_CFG_BIT_BW BIT(6)
As above, I'd suggest just defining AD7949_CFG_VAL_BW_FULL BIT(6) then it's either full or not depending on a 0 or 1 write.
Quoted 42 lines
drivers/iio/adc/ad7949.c +19
-4+#define AD7949_CFG_VAL_BW_FULL 1 +#define AD7949_CFG_VAL_BW_QUARTER 0 +#define AD7949_CFG_BIT_REF GENMASK(5, 3) +#define AD7949_CFG_BIT_SEQ GENMASK(2, 1) +#define AD7949_CFG_BIT_RBN BIT(0) +enum {ID_AD7949 =0 ,ID_AD7682 ,@@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,*/ for (i =0 ; i <2 ; i++) { ret =ad7949_spi_write_cfg (ad7949_adc, - channel <<AD7949_OFFSET_CHANNEL_SEL , -AD7949_MASK_CHANNEL_SEL ); +FIELD_PREP (AD7949_CFG_BIT_INX , channel), +AD7949_CFG_BIT_INX );if (ret)return ret;if (channel == ad7949_adc->current_channel)@@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc) {int ret;int val; +u16 cfg; -/* 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 ); + + cfg =FIELD_PREP (AD7949_CFG_BIT_CFG ,AD7949_CFG_VAL_CFG_OVERWRITE ) | +FIELD_PREP (AD7949_CFG_BIT_INCC ,AD7949_CFG_VAL_INCC_UNIPOLAR_GND ) | +FIELD_PREP (AD7949_CFG_BIT_INX , ad7949_adc->current_channel) | +FIELD_PREP (AD7949_CFG_BIT_BW ,AD7949_CFG_VAL_BW_FULL ) | +FIELD_PREP (AD7949_CFG_BIT_REF ,AD7949_REF_EXT_BUF ) | +FIELD_PREP (AD7949_CFG_BIT_SEQ ,0x0 ) | +FIELD_PREP (AD7949_CFG_BIT_RBN ,1 ); + + ret =ad7949_spi_write_cfg (ad7949_adc, cfg,AD7949_MASK_TOTAL );/* * Do two dummy conversions to apply the first configuration setting.
Re: [PATCH v2 1/4] iio: adc: ad7949: define and use bitfield names
Quoted 9 lines · Jonathan Cameron
Quoted 6 lines · On Fri, 9 Jul 2021 11:58:53 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
Replace raw configuration register values by using
FIELD_PREPand defines to improve readability.Signed-off-by Liam Beguin <[email protected]>
Hi Jonathan,
Quoted 8 lines
Ideally fixes should come before any refactors / cleanups like this one. That reduces the burden if people want to backport them.
In this particular case I'm guessing no one ran into the issues the following patches deal with so we can just take these in the order you have here.
Understood, I will follow that guideline next time.
Quoted 21 lines
Otherwise, good cleanup. A few minor comments inline, mostly as a result of some less than ideal name choices on the datasheet.
Quoted 18 lines
--- drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)drivers/iio/adc/ad7949.c +0
-3@@ -12,12 +12,27 @@#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
Quoted 3 lines
drivers/iio/adc/ad7949.c line 18
Quoted 1 line
drivers/iio/adc/ad7949.c +1
-0+#define AD7949_CFG_BIT_CFG BIT(13)Even though that's the name on the datasheet it is silly!
Agreed, datasheet register and bitfield names aren't always great :-/
Quoted 8 lines
I would have just one define called
AD7949_CFG_VAL_OVERWRITEBIT(13)It's common to do that for single flags where
FIELD_PREP(AD7949_CFG_VAL_OVERWRITE, 1)for example has an obvious meaning for the 1.
Sounds good, I'll fix these with your recommendation.
Quoted 7 lines
Quoted 7 lines
drivers/iio/adc/ad7949.c +7
-0+#define AD7949_CFG_VAL_CFG_OVERWRITE 1 +#define AD7949_CFG_VAL_CFG_KEEP 0 +#define AD7949_CFG_BIT_INCC GENMASK(12, 10) +#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND 7 +#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM 6 +#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF 4 +#define AD7949_CFG_VAL_INCC_TEMP 3
Quoted 9 lines
drivers/iio/adc/ad7949.c line 28
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+#define AD7949_CFG_VAL_INCC_BIPOLAR 2 +#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF 0 +#define AD7949_CFG_BIT_INX GENMASK(9, 7)This is rather non obvious abbreviation.
_INxwould be clearer perhaps, but then we'd get someone fixing the camel case... Given it would be good to match the datasheet, keep the name but add a comment to say this is the input channel select.
I agree! While I'm at it, I might as well add comments for INCC and others that aren't so abvious.
Thanks,
Liam
Quoted 47 lines
drivers/iio/adc/ad7949.c line 29
Quoted 1 line
drivers/iio/adc/ad7949.c +1
-0+#define AD7949_CFG_BIT_BW BIT(6)As above, I'd suggest just defining
AD7949_CFG_VAL_BW_FULLBIT(6) then it's either full or not depending on a 0 or 1 write.Quoted 42 lines
drivers/iio/adc/ad7949.c +19
-4+#define AD7949_CFG_VAL_BW_FULL 1 +#define AD7949_CFG_VAL_BW_QUARTER 0 +#define AD7949_CFG_BIT_REF GENMASK(5, 3) +#define AD7949_CFG_BIT_SEQ GENMASK(2, 1) +#define AD7949_CFG_BIT_RBN BIT(0) +enum {ID_AD7949 =0 ,ID_AD7682 ,@@ -109,8 +124,8 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,*/ for (i =0 ; i <2 ; i++) { ret =ad7949_spi_write_cfg (ad7949_adc, - channel <<AD7949_OFFSET_CHANNEL_SEL , -AD7949_MASK_CHANNEL_SEL ); +FIELD_PREP (AD7949_CFG_BIT_INX , channel), +AD7949_CFG_BIT_INX );if (ret)return ret;if (channel == ad7949_adc->current_channel)@@ -214,10 +229,19 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc) {int ret;int val; +u16 cfg; -/* 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 ); + + cfg =FIELD_PREP (AD7949_CFG_BIT_CFG ,AD7949_CFG_VAL_CFG_OVERWRITE ) | +FIELD_PREP (AD7949_CFG_BIT_INCC ,AD7949_CFG_VAL_INCC_UNIPOLAR_GND ) | +FIELD_PREP (AD7949_CFG_BIT_INX , ad7949_adc->current_channel) | +FIELD_PREP (AD7949_CFG_BIT_BW ,AD7949_CFG_VAL_BW_FULL ) | +FIELD_PREP (AD7949_CFG_BIT_REF ,AD7949_REF_EXT_BUF ) | +FIELD_PREP (AD7949_CFG_BIT_SEQ ,0x0 ) | +FIELD_PREP (AD7949_CFG_BIT_RBN ,1 ); + + ret =ad7949_spi_write_cfg (ad7949_adc, cfg,AD7949_MASK_TOTAL );/* * Do two dummy conversions to apply the first configuration setting.
PATCH v2 2/4 iio: adc: ad7949: fix spi messages on non 14-bit controllers
In reply to Liam BeguinFrom: Liam Beguin <[email protected]>
This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.
--- drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 9 deletions(-)
drivers/iio/adc/ad7949.c +60 -9
@@ -11,6 +11,7 @@#include <linux/module.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> +#include <linux/bitfield.h> #define AD7949_MASK_TOTAL GENMASK(13, 0)#define AD7949_CFG_REG_SIZE_BITS 14
@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {* @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip +* @bits_per_word: number of bits per SPI word * @cfg: copy of the configuration register * @current_channel: current channel in use * @buffer: buffer to send / receive data to / from device
@@ -67,28 +69,60 @@ struct ad7949_adc_chip {struct iio_dev *indio_dev;struct spi_device *spi;u8 resolution; +u8 bits_per_word;u16 cfg;unsigned int current_channel; -u16 buffer____cacheline_aligned ; +union { +__be16 buffer; +u8 buf8[2 ]; + }____cacheline_aligned ; }; +static void ad7949_set_bits_per_word (struct ad7949_adc_chip *ad7949_adc) +{ +u32 adc_mask =SPI_BPW_MASK (ad7949_adc->resolution); +u32 bpw = adc_mask & ad7949_adc->spi->controller->bits_per_word_mask; + +if (bpw == adc_mask) + ad7949_adc->bits_per_word = ad7949_adc->resolution; +else if (bpw ==SPI_BPW_MASK (16 )) + ad7949_adc->bits_per_word =16 ; +else + ad7949_adc->bits_per_word =8 ; +} +static int ad7949_spi_write_cfg (struct ad7949_adc_chip *ad7949_adc,u16 val,u16 mask) {int ret; -int bits_per_word = ad7949_adc->resolution; -int shift = bits_per_word -AD7949_CFG_REG_SIZE_BITS ;struct spi_message msg;struct spi_transfer tx[] = { { .tx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, }; + ad7949_adc->buffer =0 ; ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask); - ad7949_adc->buffer = ad7949_adc->cfg << shift; + +switch (ad7949_adc->bits_per_word) { +case 16 : + ad7949_adc->buffer = ad7949_adc->cfg <<2 ; +break ; +case 14 : + ad7949_adc->buffer = ad7949_adc->cfg; +break ; +case 8 : +default : +/* 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);
@@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, {int ret;int i; -int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { { .rx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, };
@@ -147,7 +179,25 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, ad7949_adc->current_channel = channel; - *val = ad7949_adc->buffer & mask; +switch (ad7949_adc->bits_per_word) { +case 16 : + *val = ad7949_adc->buffer; +/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ; +break ; +case 14 : + *val = ad7949_adc->buffer &GENMASK (13 ,0 ); +break ; +case 8 : +default : +/* 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 ; }
@@ -280,6 +330,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); ad7949_adc->vref =devm_regulator_get (dev,"vref" );if (IS_ERR (ad7949_adc->vref)) {
Signature · 1 line
2.30.1.489.g328c10930387
Jonathan Cameron
maintainerRe: [PATCH v2 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers
Quoted 7 lines · On Fri, 9 Jul 2021 11:58:54 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.
Hmm. I assumed spi controllers would all support 16 bits, but a quick grep confirms otherwise... ah well!
Quoted 28 lines
Signed-off-by Liam Beguin <[email protected]>--- drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 9 deletions(-)drivers/iio/adc/ad7949.c +2
-0@@ -11,6 +11,7 @@#include <linux/module.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> +#include <linux/bitfield.h> #define AD7949_MASK_TOTAL GENMASK(13, 0)#define AD7949_CFG_REG_SIZE_BITS 14@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {* @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip +* @bits_per_word: number of bits per SPI word * @cfg: copy of the configuration register * @current_channel: current channel in use * @buffer: buffer to send / receive data to / from device @@ -67,28 +69,60 @@ struct ad7949_adc_chip {struct iio_dev *indio_dev;
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 78
Quoted 10 lines
drivers/iio/adc/ad7949.c +5
-1struct spi_device *spi;u8 resolution; +u8 bits_per_word;u16 cfg;unsigned int current_channel; -u16 buffer____cacheline_aligned ; +union { +__be16 buffer; +u8 buf8[2 ]; + }____cacheline_aligned ;
This union made me a little nervous given issues with them under c spec, but seems that you only use buffer or buf8, there is no mixing. I think I'd rather you didn't have the union though as it makes it look like there might be.
Given we are doing cacheline spacing anyway for dma safety it take no more real space to just have
__be16 buffer ____cacheline_aligned;
buf8[2];
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 86
Quoted 8 lines
drivers/iio/adc/ad7949.c +6
-0}; +static void ad7949_set_bits_per_word (struct ad7949_adc_chip *ad7949_adc) +{ +u32 adc_mask =SPI_BPW_MASK (ad7949_adc->resolution); +u32 bpw = adc_mask & ad7949_adc->spi->controller->bits_per_word_mask; + +if (bpw == adc_mask)
if (bpw) is sufficient I think.
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 88
Quoted 2 lines
drivers/iio/adc/ad7949.c +2
-0+ ad7949_adc->bits_per_word = ad7949_adc->resolution; + else if (bpw == SPI_BPW_MASK(16))
I'm missing something here... If the resolution is 14, but the SPI controller supports 8 and 16 I'd expect us to hit here . adc_mask will not have the 16bit bit set though so bpw == 0..
else if (ad7949_adc->spi->controller_bits_per_word_mask & SPI_BPW_MASK(16))
should do the right thing.
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 91
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+ ad7949_adc->bits_per_word =16 ; + else + ad7949_adc->bits_per_word = 8;
I'd like a comment here on why this is the right default. Took me a while to think through it.
Quoted 21 lines
drivers/iio/adc/ad7949.c +4
-4+} +static int ad7949_spi_write_cfg (struct ad7949_adc_chip *ad7949_adc,u16 val,u16 mask) {int ret; -int bits_per_word = ad7949_adc->resolution; -int shift = bits_per_word -AD7949_CFG_REG_SIZE_BITS ;struct spi_message msg;struct spi_transfer tx[] = { { .tx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, }; + ad7949_adc->buffer =0 ; ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask); - ad7949_adc->buffer = ad7949_adc->cfg << shift;
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 118
Quoted 10 lines
drivers/iio/adc/ad7949.c +10
-0+ +switch (ad7949_adc->bits_per_word) { +case 16 : + ad7949_adc->buffer = ad7949_adc->cfg <<2 ; +break ; +case 14 : + ad7949_adc->buffer = ad7949_adc->cfg; +break ; +case 8 : +default :
Default here should be an error. Imagine a 12 bit device is added. Then the above set_bits_per_word() might return either 12 or 8. If it returns 12 then this is not the right thing to do. I'd argue we want to know about that rather than papering over it. Possibly we want to not let anything other than 14 bits through in set_bits_per_word() so we print an error early...
Quoted 27 lines
drivers/iio/adc/ad7949.c +8
-3+/* 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);@@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, {int ret;int i; -int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { { .rx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, };
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 184
Quoted 7 lines
drivers/iio/adc/ad7949.c +3
-1@@ -147,7 +179,25 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, ad7949_adc->current_channel = channel; - *val = ad7949_adc->buffer & mask; +switch (ad7949_adc->bits_per_word) { +case 16 : + *val = ad7949_adc->buffer;
buffer is __be16 and *val is an int so this isn't valid.
Now curious bit is I'm assuming original code worked... Given big endian platforms are rare, that makes me suspect something odd is going on.
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 187
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ;
*val >>= 16 - ad7949_adc->resolution;
No one likes conditionals if we don't need them!
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 195
Quoted 8 lines
drivers/iio/adc/ad7949.c +8
-0+break ; +case 14 : + *val = ad7949_adc->buffer &GENMASK (13 ,0 ); +break ; +case 8 : +default : +/* Convert byte array to u16, MSB first */ + *val = (ad7949_adc->buf8[0 ] <<8 ) | ad7949_adc->buf8[1 ];
Use unaligned_get_be16()
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 187
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ;
As above make this unconditional.
Quoted 13 lines
drivers/iio/adc/ad7949.c +3
-0+break ; + }return 0 ; }@@ -280,6 +330,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); ad7949_adc->vref =devm_regulator_get (dev,"vref" );if (IS_ERR (ad7949_adc->vref)) {
Re: [PATCH v2 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers
Quoted 44 lines · Jonathan Cameron
Quoted 7 lines · On Fri, 9 Jul 2021 11:58:54 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.
Hmm. I assumed spi controllers would all support 16 bits, but a quick grep confirms otherwise... ah well!
Quoted 30 lines
Signed-off-by Liam Beguin <[email protected]>--- drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 9 deletions(-)drivers/iio/adc/ad7949.c +2
-0@@ -11,6 +11,7 @@#include <linux/module.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> +#include <linux/bitfield.h> #define AD7949_MASK_TOTAL GENMASK(13, 0)#define AD7949_CFG_REG_SIZE_BITS 14@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {* @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip +* @bits_per_word: number of bits per SPI word * @cfg: copy of the configuration register * @current_channel: current channel in use * @buffer: buffer to send / receive data to / from device @@ -67,28 +69,60 @@ struct ad7949_adc_chip {struct iio_dev *indio_dev;struct spi_device *spi;u8 resolution;
Quoted 8 lines
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 78
Quoted 8 lines
drivers/iio/adc/ad7949.c +5
-1+u8 bits_per_word;u16 cfg;unsigned int current_channel; -u16 buffer____cacheline_aligned ; +union { +__be16 buffer; +u8 buf8[2 ]; + }____cacheline_aligned ;
Hi Jonathan,
Quoted 6 lines
This union made me a little nervous given issues with them under c spec, but
seems that you only use buffer or buf8, there is no mixing. I think I'd rather you didn't have the union though as it makes it look like there might be.
I don't particularly like unions either..
If I'm not mistaken, the only place where the union is useful is in the spi_transfer struct definitions. I'll use a local buf8 there instead.
Quoted 35 lines
Given we are doing cacheline spacing anyway for dma safety it take no more
real space to just have
__be16buffer____cacheline_aligned;
buf8[2];drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 86
Quoted 8 lines
drivers/iio/adc/ad7949.c +6
-0}; +static void ad7949_set_bits_per_word (struct ad7949_adc_chip *ad7949_adc) +{ +u32 adc_mask =SPI_BPW_MASK (ad7949_adc->resolution); +u32 bpw = adc_mask & ad7949_adc->spi->controller->bits_per_word_mask; + +if (bpw == adc_mask)if (bpw) is sufficient I think.
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 88
Quoted 2 lines
drivers/iio/adc/ad7949.c +2
-0+ ad7949_adc->bits_per_word = ad7949_adc->resolution; + else if (bpw == SPI_BPW_MASK(16))I'm missing something here... If the resolution is 14, but the SPI controller
supports 8 and 16 I'd expect us to hit here .adc_maskwill not have the 16bit
bit set though so bpw == 0..else if (
ad7949_adc->spi->controller_bits_per_word_mask&SPI_BPW_MASK(16))should do the right thing.
Quoted 8 lines
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 91
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+ ad7949_adc->bits_per_word =16 ; + else + ad7949_adc->bits_per_word = 8;I'd like a comment here on why this is the right default. Took me a while
to think through it.
I'll rework this function based on your comments and fail early on unsupported cases.
Quoted 32 lines
Quoted 21 lines
drivers/iio/adc/ad7949.c +4
-4+} +static int ad7949_spi_write_cfg (struct ad7949_adc_chip *ad7949_adc,u16 val,u16 mask) {int ret; -int bits_per_word = ad7949_adc->resolution; -int shift = bits_per_word -AD7949_CFG_REG_SIZE_BITS ;struct spi_message msg;struct spi_transfer tx[] = { { .tx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, }; + ad7949_adc->buffer =0 ; ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask); - ad7949_adc->buffer = ad7949_adc->cfg << shift;drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 118
Quoted 10 lines
drivers/iio/adc/ad7949.c +10
-0+ +switch (ad7949_adc->bits_per_word) { +case 16 : + ad7949_adc->buffer = ad7949_adc->cfg <<2 ; +break ; +case 14 : + ad7949_adc->buffer = ad7949_adc->cfg; +break ; +case 8 : +default :
Quoted 9 lines
Default here should be an error. Imagine a 12 bit device is added. Then the above
set_bits_per_word()might return either 12 or 8. If it returns 12 then this is not the right thing to do. I'd argue we want to know about
that rather than papering over it. Possibly we want to not let anything other
than 14 bits through inset_bits_per_word()so we print an error early...
Sorry about that, I added that default case maybe a little too quickly. I'll fix that.
Quoted 37 lines
Quoted 27 lines
drivers/iio/adc/ad7949.c +8
-3+/* 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);@@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, {int ret;int i; -int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { { .rx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, };drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 184
Quoted 7 lines
drivers/iio/adc/ad7949.c +3
-1@@ -147,7 +179,25 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, ad7949_adc->current_channel = channel; - *val = ad7949_adc->buffer & mask; +switch (ad7949_adc->bits_per_word) { +case 16 : + *val = ad7949_adc->buffer;buffer is
__be16and *val is an int so this isn't valid.
Quoted 3 lines
Now curious bit is I'm assuming original code worked... Given big endian platforms are rare, that makes me suspect something odd is going on.
Hum... that's probably my fault. I changed the type of buffer from u16 to _be16.
I'll add calls to be16_to_cpu() around this and the 14 bit case.
Quoted 7 lines
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 187
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ;*val >>= 16 -
ad7949_adc->resolution;No one likes conditionals if we don't need them!
Will do.
Quoted 13 lines
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 195
Quoted 10 lines
drivers/iio/adc/ad7949.c +8
-0+break ; +case 14 : + *val = ad7949_adc->buffer &GENMASK (13 ,0 ); +break ; +case 8 : +default : +/* Convert byte array to u16, MSB first */ + *val = (ad7949_adc->buf8[0 ] <<8 ) | ad7949_adc->buf8[1 ];Use
unaligned_get_be16()
Quoted 6 lines
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 187
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ;As above make this unconditional.
Thanks,
Liam
Quoted 13 lines
Quoted 13 lines
drivers/iio/adc/ad7949.c +3
-0+break ; + }return 0 ; }@@ -280,6 +330,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); ad7949_adc->vref =devm_regulator_get (dev,"vref" );if (IS_ERR (ad7949_adc->vref)) {
Re: [PATCH v2 2/4] iio: adc: ad7949: fix spi messages on non 14-bit controllers
Quoted 63 lines · On Sun, 11 Jul 2021 15:58:27 -0400 "Liam Beguin" <[email protected]> wrote:
Quoted 44 lines · Jonathan Cameron
Quoted 7 lines · On Fri, 9 Jul 2021 11:58:54 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
This driver supports devices with 14-bit and 16-bit sample sizes. This is not always handled properly by spi controllers and can fail. To work around this limitation, pad samples to 16-bit and split the sample into two 8-bit messages in the event that only 8-bit messages are supported by the controller.
Hmm. I assumed spi controllers would all support 16 bits, but a quick grep confirms otherwise... ah well!
Quoted 30 lines
Signed-off-by Liam Beguin <[email protected]>--- drivers/iio/adc/ad7949.c | 69 ++++++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 9 deletions(-)drivers/iio/adc/ad7949.c +2
-0@@ -11,6 +11,7 @@#include <linux/module.h> #include <linux/regulator/consumer.h> #include <linux/spi/spi.h> +#include <linux/bitfield.h> #define AD7949_MASK_TOTAL GENMASK(13, 0)#define AD7949_CFG_REG_SIZE_BITS 14@@ -57,6 +58,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {* @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip +* @bits_per_word: number of bits per SPI word * @cfg: copy of the configuration register * @current_channel: current channel in use * @buffer: buffer to send / receive data to / from device @@ -67,28 +69,60 @@ struct ad7949_adc_chip {struct iio_dev *indio_dev;struct spi_device *spi;u8 resolution;Quoted 8 lines
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 78
Quoted 8 lines
drivers/iio/adc/ad7949.c +5
-1+u8 bits_per_word;u16 cfg;unsigned int current_channel; -u16 buffer____cacheline_aligned ; +union { +__be16 buffer; +u8 buf8[2 ]; + }____cacheline_aligned ;Hi Jonathan,
Quoted 6 lines
This union made me a little nervous given issues with them under c spec, but
seems that you only use buffer or buf8, there is no mixing. I think I'd rather you didn't have the union though as it makes it look like there might be.
Quoted 4 lines
I don't particularly like unions either..
If I'm not mistaken, the only place where the union is useful is in thespi_transferstruct definitions. I'll use a local buf8 there instead.
Make sure it's either DMA safe, or that we can be sure it doesn't need to be for some reason.
Quoted 130 lines
Quoted 36 lines
Given we are doing cacheline spacing anyway for dma safety it take no more
real space to just have
__be16buffer____cacheline_aligned;
buf8[2];drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 86
Quoted 8 lines
drivers/iio/adc/ad7949.c +6
-0}; +static void ad7949_set_bits_per_word (struct ad7949_adc_chip *ad7949_adc) +{ +u32 adc_mask =SPI_BPW_MASK (ad7949_adc->resolution); +u32 bpw = adc_mask & ad7949_adc->spi->controller->bits_per_word_mask; + +if (bpw == adc_mask)if (bpw) is sufficient I think.
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 88
Quoted 2 lines
drivers/iio/adc/ad7949.c +2
-0+ ad7949_adc->bits_per_word = ad7949_adc->resolution; + else if (bpw == SPI_BPW_MASK(16))I'm missing something here... If the resolution is 14, but the SPI controller
supports 8 and 16 I'd expect us to hit here .adc_maskwill not have the 16bit
bit set though so bpw == 0..else if (
ad7949_adc->spi->controller_bits_per_word_mask&SPI_BPW_MASK(16))should do the right thing.
Quoted 8 lines
drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 91
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+ ad7949_adc->bits_per_word =16 ; + else + ad7949_adc->bits_per_word = 8;I'd like a comment here on why this is the right default. Took me a while
to think through it.I'll rework this function based on your comments and fail early on unsupported cases.
Quoted 32 lines
Quoted 21 lines
drivers/iio/adc/ad7949.c +4
-4+} +static int ad7949_spi_write_cfg (struct ad7949_adc_chip *ad7949_adc,u16 val,u16 mask) {int ret; -int bits_per_word = ad7949_adc->resolution; -int shift = bits_per_word -AD7949_CFG_REG_SIZE_BITS ;struct spi_message msg;struct spi_transfer tx[] = { { .tx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, }; + ad7949_adc->buffer =0 ; ad7949_adc->cfg = (val & mask) | (ad7949_adc->cfg & ~mask); - ad7949_adc->buffer = ad7949_adc->cfg << shift;drivers/iio/adc/ad7949.c struct ad7949_adc_chip { · line 118
Quoted 10 lines
drivers/iio/adc/ad7949.c +10
-0+ +switch (ad7949_adc->bits_per_word) { +case 16 : + ad7949_adc->buffer = ad7949_adc->cfg <<2 ; +break ; +case 14 : + ad7949_adc->buffer = ad7949_adc->cfg; +break ; +case 8 : +default :Quoted 9 lines
Default here should be an error. Imagine a 12 bit device is added. Then the above
set_bits_per_word()might return either 12 or 8. If it returns 12 then this is not the right thing to do. I'd argue we want to know about
that rather than papering over it. Possibly we want to not let anything other
than 14 bits through inset_bits_per_word()so we print an error early...Sorry about that, I added that default case maybe a little too quickly. I'll fix that.
Quoted 37 lines
Quoted 27 lines
drivers/iio/adc/ad7949.c +8
-3+/* 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);@@ -105,14 +139,12 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, {int ret;int i; -int bits_per_word = ad7949_adc->resolution; -int mask =GENMASK (ad7949_adc->resolution -1 ,0 );struct spi_message msg;struct spi_transfer tx[] = { { .rx_buf = &ad7949_adc->buffer, .len =2 , - .bits_per_word = bits_per_word, + .bits_per_word = ad7949_adc->bits_per_word, }, };drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 184
Quoted 7 lines
drivers/iio/adc/ad7949.c +3
-1@@ -147,7 +179,25 @@ static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, ad7949_adc->current_channel = channel; - *val = ad7949_adc->buffer & mask; +switch (ad7949_adc->bits_per_word) { +case 16 : + *val = ad7949_adc->buffer;buffer is
__be16and *val is an int so this isn't valid.
Quoted 7 lines
Quoted 3 lines
Now curious bit is I'm assuming original code worked... Given big endian platforms are rare, that makes me suspect something odd is going on.
Hum... that's probably my fault. I changed the type of buffer from u16 to
_be16.
I'll add calls tobe16_to_cpu()around this and the 14 bit case.
On most platforms that's going to reverse the bytes. So if this worked before without it, then it will now be broken. That makes me wonder if it was either broken previously, or the only user was a big endian host, or more likely the byte reverse isn't needed, meaning we have a wrong assumption here somewhere.
I might be missing another option though!
Jonathan
Quoted 48 lines
Quoted 8 lines
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 187
Quoted 4 lines
drivers/iio/adc/ad7949.c +3
-0+/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ;*val >>= 16 -
ad7949_adc->resolution;No one likes conditionals if we don't need them!
Will do.
Quoted 13 lines
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 195
Quoted 10 lines
drivers/iio/adc/ad7949.c +8
-0+break ; +case 14 : + *val = ad7949_adc->buffer &GENMASK (13 ,0 ); +break ; +case 8 : +default : +/* Convert byte array to u16, MSB first */ + *val = (ad7949_adc->buf8[0 ] <<8 ) | ad7949_adc->buf8[1 ];Use
unaligned_get_be16()Quoted 6 lines
drivers/iio/adc/ad7949.c static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val, · line 187
Quoted 3 lines
drivers/iio/adc/ad7949.c +3
-0+/* Shift-out padding bits */ +if (ad7949_adc->resolution ==14 ) + *val = *val >>2 ;As above make this unconditional.
Thanks,
LiamQuoted 13 lines
Quoted 13 lines
drivers/iio/adc/ad7949.c +3
-0+break ; + }return 0 ; }@@ -280,6 +330,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); ad7949_adc->vref =devm_regulator_get (dev,"vref" );if (IS_ERR (ad7949_adc->vref)) {
PATCH v2 3/4 iio: adc: ad7949: add support for internal vref
In reply to Liam BeguinFrom: Liam Beguin <[email protected]>
Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.
--- drivers/iio/adc/ad7949.c | 94 ++++++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 14 deletions(-)
drivers/iio/adc/ad7949.c +80 -14
@@ -31,6 +31,7 @@#define AD7949_CFG_VAL_BW_FULL 1#define AD7949_CFG_VAL_BW_QUARTER 0#define AD7949_CFG_BIT_REF GENMASK(5, 3) +#define AD7949_CFG_VAL_REF_EXTERNAL BIT(1)#define AD7949_CFG_BIT_SEQ GENMASK(2, 1)#define AD7949_CFG_BIT_RBN BIT(0)
@@ -40,6 +41,33 @@ enum { ID_AD7689, }; +/** +* enum ad7949_ref - Reference selection +* +* AD7949_REF_INT_2500: Internal reference and temperature sensor enabled. +* Vref=2.5V, buffered output +* AD7949_REF_INT_4096: Internal reference and temperature sensor enabled. +* Vref=4.096V, buffered output +* AD7949_REF_EXT_TEMP: Use external reference, temperature sensor enabled. +* Internal buffer disabled +* AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and +* temperature sensor enabled. +* AD7949_REF_RSRV_4: Do not use +* AD7949_REF_RSRV_5: Do not use +* AD7949_REF_EXT: Use external reference, internal buffer and +* temperature sensor disabled. +* AD7949_REF_EXT_BUF: Use external reference, internal buffer enabled. +* Internal reference and temperature sensor disabled. +*/ +enum ad7949_ref { +AD7949_REF_INT_2500 =0 , +AD7949_REF_INT_4096 , +AD7949_REF_EXT_TEMP , +AD7949_REF_EXT_TEMP_BUF , +AD7949_REF_EXT =6 , +AD7949_REF_EXT_BUF , +}; +struct ad7949_adc_spec {u8 num_channels;u8 resolution;
@@ -55,6 +83,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {* struct ad7949_adc_chip - AD ADC chip * @lock: protects write sequences * @vref: regulator generating Vref +* @refsel: reference selection * @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip
@@ -66,6 +95,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {struct ad7949_adc_chip {struct mutex lock;struct regulator *vref; +enum ad7949_ref refsel;struct iio_dev *indio_dev;struct spi_device *spi;u8 resolution;
@@ -243,12 +273,28 @@ 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; +switch (ad7949_adc->refsel) { +case AD7949_REF_INT_2500 : + *val =2500 ; +break ; +case AD7949_REF_INT_4096 : + *val =4096 ; +break ; +case AD7949_REF_EXT_TEMP : +case AD7949_REF_EXT_TEMP_BUF : +case AD7949_REF_EXT : +case AD7949_REF_EXT_BUF : + ret =regulator_get_voltage (ad7949_adc->vref); +if (ret <0 ) +return ret; + +/* convert value back to mV */ + *val = ret /1000 ; +break ; + } - *val = ret /5000 ; -return IIO_VAL_INT ; + *val2 = (1 << ad7949_adc->resolution) -1 ; +return IIO_VAL_FRACTIONAL ; }return -EINVAL ;
@@ -287,7 +333,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)FIELD_PREP (AD7949_CFG_BIT_INCC ,AD7949_CFG_VAL_INCC_UNIPOLAR_GND ) |FIELD_PREP (AD7949_CFG_BIT_INX , ad7949_adc->current_channel) |FIELD_PREP (AD7949_CFG_BIT_BW ,AD7949_CFG_VAL_BW_FULL ) | -FIELD_PREP (AD7949_CFG_BIT_REF ,AD7949_REF_EXT_BUF ) | +FIELD_PREP (AD7949_CFG_BIT_REF , ad7949_adc->refsel) |FIELD_PREP (AD7949_CFG_BIT_SEQ ,0x0 ) |FIELD_PREP (AD7949_CFG_BIT_RBN ,1 );
@@ -306,6 +352,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)static int ad7949_spi_probe (struct spi_device *spi) {struct device *dev = &spi->dev; +struct device_node *np = dev->of_node;const struct ad7949_adc_spec *spec;struct ad7949_adc_chip *ad7949_adc;struct iio_dev *indio_dev;
@@ -332,16 +379,35 @@ static int ad7949_spi_probe(struct spi_device *spi) ad7949_adc->resolution = spec->resolution;ad7949_set_bits_per_word (ad7949_adc); - 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); +/* Set default devicetree parameters */ + ad7949_adc->refsel =AD7949_REF_EXT_BUF ; +of_property_read_u32 (np,"adi,reference-select" , &ad7949_adc->refsel); +switch (ad7949_adc->refsel) { +case AD7949_REF_INT_2500 : +case AD7949_REF_INT_4096 : +case AD7949_REF_EXT_TEMP : +case AD7949_REF_EXT_TEMP_BUF : +case AD7949_REF_EXT : +case AD7949_REF_EXT_BUF : +break ; +default : +dev_err (dev,"invalid adi,reference-select value (%d)\n" , + ad7949_adc->refsel); +return -EINVAL ; } - ret =regulator_enable (ad7949_adc->vref); -if (ret <0 ) { -dev_err (dev,"fail to enable regulator\n" ); -return ret; +if (ad7949_adc->refsel &AD7949_CFG_VAL_REF_EXTERNAL ) { + 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);
Signature · 1 line
2.30.1.489.g328c10930387
Jonathan Cameron
maintainerRe: [PATCH v2 3/4] iio: adc: ad7949: add support for internal vref
Quoted 37 lines · On Fri, 9 Jul 2021 11:58:55 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
Add support for selecting a custom reference voltage from the devicetree. If an external source is used, a vref regulator should be defined in the devicetree.
Signed-off-by Liam Beguin <[email protected]>--- drivers/iio/adc/ad7949.c | 94 ++++++++++++++++++++++++++++++++++------ 1 file changed, 80 insertions(+), 14 deletions(-)drivers/iio/adc/ad7949.c +11
-0@@ -31,6 +31,7 @@#define AD7949_CFG_VAL_BW_FULL 1#define AD7949_CFG_VAL_BW_QUARTER 0#define AD7949_CFG_BIT_REF GENMASK(5, 3) +#define AD7949_CFG_VAL_REF_EXTERNAL BIT(1)#define AD7949_CFG_BIT_SEQ GENMASK(2, 1)#define AD7949_CFG_BIT_RBN BIT(0)@@ -40,6 +41,33 @@ enum { ID_AD7689, }; +/** +* enum ad7949_ref - Reference selection +* +* AD7949_REF_INT_2500: Internal reference and temperature sensor enabled. +* Vref=2.5V, buffered output +* AD7949_REF_INT_4096: Internal reference and temperature sensor enabled. +* Vref=4.096V, buffered output +* AD7949_REF_EXT_TEMP: Use external reference, temperature sensor enabled. +* Internal buffer disabled +* AD7949_REF_EXT_TEMP_BUF: Use external reference, internal buffer and
drivers/iio/adc/ad7949.c enum { · line 63
Quoted 10 lines
drivers/iio/adc/ad7949.c +10
-0+* temperature sensor enabled. +* AD7949_REF_RSRV_4: Do not use +* AD7949_REF_RSRV_5: Do not use +* AD7949_REF_EXT: Use external reference, internal buffer and +* temperature sensor disabled. +* AD7949_REF_EXT_BUF: Use external reference, internal buffer enabled. +* Internal reference and temperature sensor disabled. +*/ +enum ad7949_ref { +AD7949_REF_INT_2500 =0 ,
For case where you have gaps like this, an ENUM is probably not appropriate. I'd just used defines.
Quoted 78 lines
drivers/iio/adc/ad7949.c +32
-6+ AD7949_REF_INT_4096, +AD7949_REF_EXT_TEMP , +AD7949_REF_EXT_TEMP_BUF , +AD7949_REF_EXT =6 , + AD7949_REF_EXT_BUF, +}; +struct ad7949_adc_spec {u8 num_channels;u8 resolution;@@ -55,6 +83,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {* struct ad7949_adc_chip - AD ADC chip * @lock: protects write sequences * @vref: regulator generating Vref +* @refsel: reference selection * @indio_dev: reference to iio structure * @spi: reference to spi structure * @resolution: resolution of the chip @@ -66,6 +95,7 @@ static const struct ad7949_adc_spec ad7949_adc_spec[] = {struct ad7949_adc_chip {struct mutex lock;struct regulator *vref; +enum ad7949_ref refsel;struct iio_dev *indio_dev;struct spi_device *spi;u8 resolution;@@ -243,12 +273,28 @@ 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; +switch (ad7949_adc->refsel) { +case AD7949_REF_INT_2500 : + *val =2500 ; +break ; +case AD7949_REF_INT_4096 : + *val =4096 ; +break ; +case AD7949_REF_EXT_TEMP : +case AD7949_REF_EXT_TEMP_BUF : +case AD7949_REF_EXT : +case AD7949_REF_EXT_BUF : + ret =regulator_get_voltage (ad7949_adc->vref); +if (ret <0 ) +return ret; + +/* convert value back to mV */ + *val = ret /1000 ; +break ; + } - *val = ret /5000 ; -return IIO_VAL_INT ; + *val2 = (1 << ad7949_adc->resolution) -1 ; +return IIO_VAL_FRACTIONAL ; }return -EINVAL ;@@ -287,7 +333,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)FIELD_PREP (AD7949_CFG_BIT_INCC ,AD7949_CFG_VAL_INCC_UNIPOLAR_GND ) |FIELD_PREP (AD7949_CFG_BIT_INX , ad7949_adc->current_channel) |FIELD_PREP (AD7949_CFG_BIT_BW ,AD7949_CFG_VAL_BW_FULL ) | -FIELD_PREP (AD7949_CFG_BIT_REF ,AD7949_REF_EXT_BUF ) | +FIELD_PREP (AD7949_CFG_BIT_REF , ad7949_adc->refsel) |FIELD_PREP (AD7949_CFG_BIT_SEQ ,0x0 ) |FIELD_PREP (AD7949_CFG_BIT_RBN ,1 );@@ -306,6 +352,7 @@ static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)static int ad7949_spi_probe (struct spi_device *spi) {struct device *dev = &spi->dev; +struct device_node *np = dev->of_node;const struct ad7949_adc_spec *spec;struct ad7949_adc_chip *ad7949_adc;struct iio_dev *indio_dev;
drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 384
Quoted 10 lines
drivers/iio/adc/ad7949.c +3
-4@@ -332,16 +379,35 @@ static int ad7949_spi_probe(struct spi_device *spi) ad7949_adc->resolution = spec->resolution;ad7949_set_bits_per_word (ad7949_adc); - 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); +/* Set default devicetree parameters */ + ad7949_adc->refsel =AD7949_REF_EXT_BUF ; +of_property_read_u32 (np,"adi,reference-select" , &ad7949_adc->refsel);
Check for errors. Not being present is fine, but other errors may not be.
Quoted 16 lines
drivers/iio/adc/ad7949.c +12
-2+switch (ad7949_adc->refsel) { +case AD7949_REF_INT_2500 : +case AD7949_REF_INT_4096 : +case AD7949_REF_EXT_TEMP : +case AD7949_REF_EXT_TEMP_BUF : +case AD7949_REF_EXT : +case AD7949_REF_EXT_BUF : +break ; +default : +dev_err (dev,"invalid adi,reference-select value (%d)\n" , + ad7949_adc->refsel); +return -EINVAL ; } - ret =regulator_enable (ad7949_adc->vref); -if (ret <0 ) {
drivers/iio/adc/ad7949.c static int ad7949_spi_probe(struct spi_device *spi) · line 406
Quoted 10 lines
drivers/iio/adc/ad7949.c +8
-2-dev_err (dev,"fail to enable regulator\n" ); -return ret; +if (ad7949_adc->refsel &AD7949_CFG_VAL_REF_EXTERNAL ) { + 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);
Shouldn't be calling regulator_disable unless you enabled it...
Quoted 7 lines
drivers/iio/adc/ad7949.c +4
-0+if (ret <0 ) { +dev_err (dev,"fail to enable regulator\n" ); +return ret; + } }mutex_init (&ad7949_adc->lock);
PATCH v2 4/4 dt-bindings: iio: adc: ad7949: add adi,reference-source
In reply to Liam BeguinFrom: Liam Beguin <[email protected]>
Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.
--- .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +21 -0
@@ -35,6 +35,27 @@ properties: "#io-channel-cells": const: 1 + adi,reference-select: + description: | + Select the reference voltage source to use when converting samples. + Acceptable values are: + - 0: Internal reference and temperature sensor enabled. + Vref=2.5V, buffered output + - 1: Internal reference and temperature sensor enabled. + Vref=4.096V, buffered output + - 2: Use external reference, temperature sensor enabled. + Internal buffer disabled + - 3: Use external reference, internal buffer and temperature sensor + enabled. + - 6: Use external reference, internal buffer and temperature sensor + disabled. + - 7: Use external reference, internal buffer enabled. + Internal reference and temperature sensor disabled. + + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3, 6, 7] + default: 7 + required: - compatible - reg
Signature · 1 line
2.30.1.489.g328c10930387
Jonathan Cameron
maintainerRe: [PATCH v2 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source
Quoted 5 lines · On Fri, 9 Jul 2021 11:58:56 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.
Should this be per channel? That will effect some of what I say below...
Quoted 11 lines
Signed-off-by Liam Beguin <[email protected]>--- .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +0
-0
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 38
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +1
-0@@ -35,6 +35,27 @@ properties: "#io-channel-cells": const: 1 + adi,reference-select:
This is one field in the register, but it's not one thing, so lets break it up in DT. We should do this both to make for more readable dts files and to enforce the requirements on regulators...
Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +5
-0+ description: | + Select the reference voltage source to use when converting samples. + Acceptable values are: + - 0: Internal reference and temperature sensor enabled. + Vref=2.5V, buffered output
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 53
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +10
-0+ - 1: Internal reference and temperature sensor enabled. + Vref=4.096V, buffered output + - 2: Use external reference, temperature sensor enabled. + Internal buffer disabled + - 3: Use external reference, internal buffer and temperature sensor + enabled. + - 6: Use external reference, internal buffer and temperature sensor + disabled. + - 7: Use external reference, internal buffer enabled. + Internal reference and temperature sensor disabled.
So question 1 is whether to use an external or internal reference. Normally we'd make the coarse decision of whether to use an external reference by whether there is a regulator provided. That won't work so well if we make this per channel.
Question 2, assuming internal reference, what voltage? Those should take an actual voltage (probably in mV and match against an enum of the two possible values). Binding should check to make sure this isn't specified as well as saying we are using an external refernce.
Question 3, assuming external reference, is temperature sensor enabled?
- actually dumb question, but why would anyone not want this enabled? Maybe turn it off in runtime pm, but in general if you've fitted a chip with a temperature sensor you at least sometimes want to measure temperature! So my gut feeling is don't allow this to be controlled (effectively drop cases 6 and 7 above as being unlikely to be of interest to anyone)
Question 4, Is the internal buffer enabled when using and external reference.
This one is interesting. We could just expose it in general, but I wonder
if we can do something that reflects how it is used. From the various figures in the datasheet this seems to be coupled to whether the external reference is on pin REF_IN or pin REF. If that's the case can we have two optional regs only one of which should be supplied? However, this gets more fiddly because the default right now is vref-supply actually being connected to the vrefin connection. That's annoying as it stops us using the obvious naming...
Hence I think we can have
vref-supply (actually connected to vrefin) and vref-unbuffered-supply
Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +5
-0+ + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3, 6, 7] + default: 7 + required: - compatible - reg
Re: [PATCH v2 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source
Hi Jonathan,
Quoted 11 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 9 Jul 2021 11:58:56 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.
Should this be per channel? That will effect some of what I say below...
We could make it per channel. Ideally, I'd also like to add support for differential channels, so might as well add per channel configurations now.
Quoted 51 lines
Quoted 11 lines
Signed-off-by Liam Beguin <[email protected]>--- .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +0
-0Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 38
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +1
-0@@ -35,6 +35,27 @@ properties: "#io-channel-cells": const: 1 + adi,reference-select:This is one field in the register, but it's not one thing, so lets break it up
in DT. We should do this both to make for more readable dts files and to enforce the requirements on regulators...Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +5
-0+ description: | + Select the reference voltage source to use when converting samples. + Acceptable values are: + - 0: Internal reference and temperature sensor enabled. + Vref=2.5V, buffered outputDocumentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 53
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +10
-0+ - 1: Internal reference and temperature sensor enabled. + Vref=4.096V, buffered output + - 2: Use external reference, temperature sensor enabled. + Internal buffer disabled + - 3: Use external reference, internal buffer and temperature sensor + enabled. + - 6: Use external reference, internal buffer and temperature sensor + disabled. + - 7: Use external reference, internal buffer enabled. + Internal reference and temperature sensor disabled.So question 1 is whether to use an external or internal reference. Normally we'd make the coarse decision of whether to use an external reference
by whether there is a regulator provided. That won't work so well if we make
this per channel.Question 2, assuming internal reference, what voltage? Those should take an actual voltage (probably in mV and match against an enum of the two possible values).
Binding should check to make sure this isn't specified as well as saying we
are using an external refernce.
Quoted 11 lines
Question 3, assuming external reference, is temperature sensor enabled?
- actually dumb question, but why would anyone not want this enabled? Maybe turn it
off in runtime pm, but in general if you've fitted a chip with a temperature sensor
you at least sometimes want to measure temperature! So my gut feeling is don't
allow this to be controlled (effectively drop cases 6 and 7 above as being
unlikely to be of interest to anyone)
I like your suggestion of breaking this down so far, it would look something like this:
ad7949: adc@0 {
compatible = "adi,ad7949";
reg = <0>;
vref-supply = <&vdd_supply>;
channel@0 {
adi,internal-ref-mv = <2500>;
reg = <0>;
};
channel@1 {
reg = <1>;
/*
* defaults to vref-supply if defined or error
* out
*/
};
};Quoted 8 lines
Question 4, Is the internal buffer enabled when using and external reference.
This one is interesting. We could just expose it in general, but I wonder
if we can do something that reflects how it is used. From the various figures in
the datasheet this seems to be coupled to whether the external reference is on
Quoted 9 lines
pin
REF_INor pin REF. If that's the case can we have two optional regs only
one of which should be supplied? However, this gets more fiddly because the default right now is vref-supply actually being connected to the vrefin connection.
That's annoying as it stops us using the obvious naming... Hence I think we can have
vref-supply (actually connected to vrefin) and vref-unbuffered-supply
I really like the idea of using the same names as the datasheet (vref-supply and vrefin-supply), to infer the buffered state, but it's annoying (and confusing) that it's setup the other way right now.
I wonder what happens if the reference is connected to refin and we're configured as unbuffered (and the other way around).
I looked around and I might be able to test it on one setup I have where the external reference is connected to REF.
If it's not a breaking change, would it be okay with you to follow the datasheet naming?
Liam
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +5
-0+ + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3, 6, 7] + default: 7 + required: - compatible - reg
Re: [PATCH v2 4/4] dt-bindings: iio: adc: ad7949: add adi,reference-source
Quoted 128 lines · On Mon, 12 Jul 2021 13:05:23 -0400 "Liam Beguin" <[email protected]> wrote:
Hi Jonathan,
Quoted 11 lines · Jonathan Cameron
Quoted 5 lines · On Fri, 9 Jul 2021 11:58:56 -0400 Liam Beguin <[email protected]> wrote:
From: Liam Beguin <[email protected]>
Add bindings documentation for the adi,reference-source property. This property is required to properly configure the ADC sample request based on which reference source should be used for the calculation.
Should this be per channel? That will effect some of what I say below...
We could make it per channel. Ideally, I'd also like to add support for differential channels, so might as well add per channel configurations now.
Quoted 51 lines
Quoted 11 lines
Signed-off-by Liam Beguin <[email protected]>--- .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+)Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +0
-0Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 38
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +1
-0@@ -35,6 +35,27 @@ properties: "#io-channel-cells": const: 1 + adi,reference-select:This is one field in the register, but it's not one thing, so lets break it up
in DT. We should do this both to make for more readable dts files and to enforce the requirements on regulators...Quoted 5 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +5
-0+ description: | + Select the reference voltage source to use when converting samples. + Acceptable values are: + - 0: Internal reference and temperature sensor enabled. + Vref=2.5V, buffered outputDocumentation/devicetree/bindings/iio/adc/adi,ad7949.yaml properties: · line 53
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +10
-0+ - 1: Internal reference and temperature sensor enabled. + Vref=4.096V, buffered output + - 2: Use external reference, temperature sensor enabled. + Internal buffer disabled + - 3: Use external reference, internal buffer and temperature sensor + enabled. + - 6: Use external reference, internal buffer and temperature sensor + disabled. + - 7: Use external reference, internal buffer enabled. + Internal reference and temperature sensor disabled.So question 1 is whether to use an external or internal reference. Normally we'd make the coarse decision of whether to use an external reference
by whether there is a regulator provided. That won't work so well if we make
this per channel.Question 2, assuming internal reference, what voltage? Those should take an actual voltage (probably in mV and match against an enum of the two possible values).
Binding should check to make sure this isn't specified as well as saying we
are using an external refernce.Quoted 11 lines
Question 3, assuming external reference, is temperature sensor enabled?
- actually dumb question, but why would anyone not want this enabled? Maybe turn it
off in runtime pm, but in general if you've fitted a chip with a temperature sensor
you at least sometimes want to measure temperature! So my gut feeling is don't
allow this to be controlled (effectively drop cases 6 and 7 above as being
unlikely to be of interest to anyone)I like your suggestion of breaking this down so far, it would look something like this:
ad7949: adc@0 { compatible = "adi,ad7949"; reg = <0>; vref-supply = <&vdd_supply>; channel@0 { adi,internal-ref-mv = <2500>; reg = <0>; }; channel@1 { reg = <1>; /* * defaults to vref-supply if defined or error * out */ }; };Quoted 8 lines
Question 4, Is the internal buffer enabled when using and external reference.
This one is interesting. We could just expose it in general, but I wonder
if we can do something that reflects how it is used. From the various figures in
the datasheet this seems to be coupled to whether the external reference is onQuoted 9 lines
pin
REF_INor pin REF. If that's the case can we have two optional regs only
one of which should be supplied? However, this gets more fiddly because the default right now is vref-supply actually being connected to the vrefin connection.
That's annoying as it stops us using the obvious naming... Hence I think we can have
vref-supply (actually connected to vrefin) and vref-unbuffered-supplyI really like the idea of using the same names as the datasheet (vref-supply and vrefin-supply), to infer the buffered state, but it's annoying (and confusing) that it's setup the other way right now.
Quoted 7 lines
I wonder what happens if the reference is connected to refin and we're configured as unbuffered (and the other way around).
I looked around and I might be able to test it on one setup I have where the external reference is connected to REF.If it's not a breaking change, would it be okay with you to follow the datasheet naming?
Absolutely. If we can get away with fixing that it would be great.
Jonathan
Quoted 13 lines
Liam
Quoted 10 lines
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml +5
-0+ + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1, 2, 3, 6, 7] + default: 7 + required: - compatible - reg