| Message ID | 20210709155856.1732245-1-liambeguin@gmail.com |
|---|---|
| Headers | show |
| Series | AD7949 Fixes | expand |
On Fri, 9 Jul 2021 11:58:53 -0400 Liam Beguin <liambeguin@gmail.com> wrote: > From: Liam Beguin <lvb@xiphos.com> > > Replace raw configuration register values by using FIELD_PREP and > defines to improve readability. > > Signed-off-by: Liam Beguin <lvb@xiphos.com> 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. > --- > drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++------- > 1 file changed, 31 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > index 1b4b3203e428..93aacf4f680b 100644 > --- a/drivers/iio/adc/ad7949.c > +++ b/drivers/iio/adc/ad7949.c > @@ -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 called AD7949_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. > +#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. > +#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. > +#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.
On Fri, 9 Jul 2021 11:58:56 -0400 Liam Beguin <liambeguin@gmail.com> wrote: > From: Liam Beguin <lvb@xiphos.com> > > 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... > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > --- > .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > index 9b56bd4d5510..eae3121cad01 100644 > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > @@ -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... > + 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. 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 > + > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2, 3, 6, 7] > + default: 7 > + > required: > - compatible > - reg
On Sat Jul 10, 2021 at 12:08 PM EDT, Jonathan Cameron wrote: > On Fri, 9 Jul 2021 11:58:53 -0400 > Liam Beguin <liambeguin@gmail.com> wrote: > > > From: Liam Beguin <lvb@xiphos.com> > > > > Replace raw configuration register values by using FIELD_PREP and > > defines to improve readability. > > > > Signed-off-by: Liam Beguin <lvb@xiphos.com> Hi Jonathan, > > 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. > Otherwise, good cleanup. A few minor comments inline, mostly as a result > of some less than ideal name choices on the datasheet. > > > --- > > drivers/iio/adc/ad7949.c | 38 +++++++++++++++++++++++++++++++------- > > 1 file changed, 31 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c > > index 1b4b3203e428..93aacf4f680b 100644 > > --- a/drivers/iio/adc/ad7949.c > > +++ b/drivers/iio/adc/ad7949.c > > @@ -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! Agreed, datasheet register and bitfield names aren't always great :-/ > > I would have just one define called > AD7949_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. > Sounds good, I'll fix these with your recommendation. > > +#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. > I agree! While I'm at it, I might as well add comments for INCC and others that aren't so abvious. Thanks, Liam > > +#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. > > > +#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.
Hi Jonathan, On Sat Jul 10, 2021 at 1:00 PM EDT, Jonathan Cameron wrote: > On Fri, 9 Jul 2021 11:58:56 -0400 > Liam Beguin <liambeguin@gmail.com> wrote: > > > From: Liam Beguin <lvb@xiphos.com> > > > > 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. > > > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > > --- > > .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > > index 9b56bd4d5510..eae3121cad01 100644 > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > > @@ -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... > > > + 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. > > 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) > 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 */ }; }; > 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 > 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 > > > > + > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + enum: [0, 1, 2, 3, 6, 7] > > + default: 7 > > + > > required: > > - compatible > > - reg
On Mon, 12 Jul 2021 13:05:23 -0400 "Liam Beguin" <liambeguin@gmail.com> wrote: > Hi Jonathan, > > On Sat Jul 10, 2021 at 1:00 PM EDT, Jonathan Cameron wrote: > > On Fri, 9 Jul 2021 11:58:56 -0400 > > Liam Beguin <liambeguin@gmail.com> wrote: > > > > > From: Liam Beguin <lvb@xiphos.com> > > > > > > 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. > > > > > > > Signed-off-by: Liam Beguin <lvb@xiphos.com> > > > --- > > > .../bindings/iio/adc/adi,ad7949.yaml | 21 +++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > > > index 9b56bd4d5510..eae3121cad01 100644 > > > --- a/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > > > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml > > > @@ -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... > > > > > + 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. > > > > 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) > > > > 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 > */ > }; > }; > > > 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 > > > > 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? Absolutely. If we can get away with fixing that it would be great. Jonathan > > Liam > > > > > > > > + > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1, 2, 3, 6, 7] > > > + default: 7 > > > + > > > required: > > > - compatible > > > - reg
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