[PATCH v4 0/5] AD7949 Fixes
From: Liam Beguin
Date: Tue Jul 27 2021 - 19:29:28 EST
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.
V1 was 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 v3:
- use cpu_to_be16 and be16_to_cpu instead of manual conversion
- use pointers to channel structures instead of copies
- switch to generic device firmware property API
- use standard unit suffix names (mV to microvolt)
- switch to devm_iio_device_register() for additional cleanup
Changes since v2:
- add comments to ambiguous register names
- fix be16 definition of the adc buffer
- fix BPW logic
- rework vref support
- support per channel vref selection
- infer reference select value based on DT parameters
- update dt-binding
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 (5):
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 per channel reference
iio: adc: ad7949: use devm managed functions
.../bindings/iio/adc/adi,ad7949.yaml | 69 ++++-
drivers/iio/adc/ad7949.c | 261 ++++++++++++++----
2 files changed, 274 insertions(+), 56 deletions(-)
Range-diff against v3:
-: ------------ > 1: 8760b368f971 iio: adc: ad7949: define and use bitfield names
1: a9243c834705 ! 2: 7b1484f2fc4c iio: adc: ad7949: fix spi messages on non 14-bit controllers
@@ Commit message
Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx>
## drivers/iio/adc/ad7949.c ##
-@@
- #include <linux/regulator/consumer.h>
- #include <linux/spi/spi.h>
- #include <linux/bitfield.h>
-+#include <asm/unaligned.h>
-
- #define AD7949_MASK_TOTAL GENMASK(13, 0)
-
@@ drivers/iio/adc/ad7949.c: static const struct ad7949_adc_spec ad7949_adc_spec[] = {
* @indio_dev: reference to iio structure
* @spi: reference to spi structure
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_write_cfg(struct ad7949_adc_chip
int ret;
- int bits_per_word = ad7949_adc->resolution;
- int shift = bits_per_word - AD7949_CFG_REG_SIZE_BITS;
-+ u8 buf8[2];
struct spi_message msg;
struct spi_transfer tx[] = {
{
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_write_cfg(struct ad7949_adc_chip
+ ad7949_adc->buffer = ad7949_adc->cfg;
+ break;
+ case 8:
-+ /* Pack 14-bit value into 2 bytes, MSB first */
-+ buf8[0] = FIELD_GET(GENMASK(13, 6), ad7949_adc->cfg);
-+ buf8[1] = FIELD_GET(GENMASK(5, 0), ad7949_adc->cfg) << 2;
-+ memcpy(&ad7949_adc->buffer, buf8, 2);
++ /* Here, type is big endian as it must be sent in two transfers */
++ ad7949_adc->buffer = (u16)cpu_to_be16(ad7949_adc->cfg << 2);
+ break;
+ default:
+ dev_err(&ad7949_adc->indio_dev->dev, "unsupported BPW\n");
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
int i;
- int bits_per_word = ad7949_adc->resolution;
- int mask = GENMASK(ad7949_adc->resolution - 1, 0);
-+ u8 buf8[2];
struct spi_message msg;
struct spi_transfer tx[] = {
{
@@ 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:
-+ memcpy(buf8, &ad7949_adc->buffer, 2);
-+ /* Convert byte array to u16, MSB first */
-+ *val = get_unaligned_be16(buf8);
++ /* Here, type is big endian as data was sent in two transfers */
++ *val = be16_to_cpu(ad7949_adc->buffer);
+ /* Shift-out padding bits */
+ *val >>= 16 - ad7949_adc->resolution;
+ break;
2: bb25b7fcb0a3 ! 3: 41c4ab9c5e19 iio: adc: ad7949: add support for internal vref
@@ drivers/iio/adc/ad7949.c: struct ad7949_adc_chip {
u8 bits_per_word;
u16 cfg;
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
+ int ret;
int i;
- u8 buf8[2];
struct spi_message msg;
-+ struct ad7949_channel ad7949_chan = ad7949_adc->channels[channel];
++ struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[channel];
struct spi_transfer tx[] = {
{
.rx_buf = &ad7949_adc->buffer,
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_channel(struct ad7949_adc_c
- FIELD_PREP(AD7949_CFG_BIT_INX, channel),
- AD7949_CFG_BIT_INX);
+ FIELD_PREP(AD7949_CFG_BIT_INX, channel) |
-+ FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan.refsel),
++ FIELD_PREP(AD7949_CFG_BIT_REF, ad7949_chan->refsel),
+ AD7949_CFG_BIT_INX | AD7949_CFG_BIT_REF);
if (ret)
return ret;
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_raw(struct iio_dev *indio_d
int *val, int *val2, long mask)
{
struct ad7949_adc_chip *ad7949_adc = iio_priv(indio_dev);
-+ struct ad7949_channel ad7949_chan = ad7949_adc->channels[chan->channel];
++ struct ad7949_channel *ad7949_chan = &ad7949_adc->channels[chan->channel];
int ret;
if (!val)
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_read_raw(struct iio_dev *indio_d
- ret = regulator_get_voltage(ad7949_adc->vref);
- if (ret < 0)
- return ret;
-+ switch (ad7949_chan.refsel) {
++ switch (ad7949_chan->refsel) {
+ case AD7949_CFG_VAL_REF_INT_2500:
+ *val = 2500;
+ break;
@@ 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 ad7949_channel *ad7949_chan;
-+ struct device_node *child;
++ struct fwnode_handle *child;
struct iio_dev *indio_dev;
+ int mode;
+ u32 tmp;
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
- if (ret < 0) {
- dev_err(dev, "fail to enable regulator\n");
- return ret;
-+ if (mode > AD7949_CFG_VAL_REF_INT_4096) {
++ if (mode & AD7949_CFG_VAL_REF_EXTERNAL) {
+ ret = regulator_enable(ad7949_adc->vref);
+ if (ret < 0) {
+ dev_err(dev, "fail to enable regulator\n");
@@ drivers/iio/adc/ad7949.c: static int ad7949_spi_probe(struct spi_device *spi)
+ }
+
+ /* Initialize all channel structures */
-+ for (i = 0; i < spec->num_channels; i++) {
++ for (i = 0; i < spec->num_channels; i++)
+ ad7949_adc->channels[i].refsel = mode;
-+ }
+
+ /* Read channel specific information form the devicetree */
-+ for_each_child_of_node(dev->of_node, child) {
-+ ret = of_property_read_u32(child, "reg", &i);
++ device_for_each_child_node(dev, child) {
++ ret = fwnode_property_read_u32(child, "reg", &i);
+ if (ret) {
-+ dev_err(dev, "missing reg property in child: %s\n",
-+ child->full_name);
-+ of_node_put(child);
++ dev_err(dev, "missing reg property in %pfw\n", child);
++ fwnode_handle_put(child);
+ return ret;
+ }
+
+ ad7949_chan = &ad7949_adc->channels[i];
+
-+ ret = of_property_read_u32(child, "adi,internal-ref-mv", &tmp);
++ ret = fwnode_property_read_u32(child, "adi,internal-ref-microvolt", &tmp);
+ if (ret < 0 && ret != -EINVAL) {
-+ of_node_put(child);
++ dev_err(dev, "invalid internal reference in %pfw\n", child);
++ fwnode_handle_put(child);
+ return ret;
+ }
+
+ switch (tmp) {
-+ case 2500:
++ case 2500000:
+ ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_2500;
+ break;
-+ case 4096:
++ case 4096000:
+ ad7949_chan->refsel = AD7949_CFG_VAL_REF_INT_4096;
+ break;
+ default:
+ dev_err(dev, "unsupported internal voltage reference\n");
-+ of_node_put(child);
++ fwnode_handle_put(child);
+ return -EINVAL;
+ }
}
3: 41e3ca4f0d52 ! 4: 9cb48acbd05b dt-bindings: iio: adc: ad7949: add per channel reference
@@ Commit message
Add bindings documentation describing per channel reference voltage
selection.
- This adds the adi,internal-ref-mv property, and child nodes for each
- channel. This is required to properly configure the ADC sample request
- based on which reference source should be used for the calculation.
+ This adds the adi,internal-ref-microvolt property, and child nodes for
+ each channel. This is required to properly configure the ADC sample
+ request based on which reference source should be used for the
+ calculation.
Signed-off-by: Liam Beguin <lvb@xxxxxxxxxx>
@@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
+
+ '#size-cells':
+ const: 0
-+
+
required:
- compatible
@@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
+ minimum: 0
+ maximum: 7
+
-+ adi,internal-ref-mv:
++ adi,internal-ref-microvolt:
+ description: |
-+ Internal reference voltage selection in millivolts.
++ Internal reference voltage selection in microvolts.
+
+ If no internal reference is specified, the channel will default to the
+ external reference defined by vrefin-supply (or vref-supply).
@@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: properties:
+ If no supplies are defined, the reference selection will default to
+ 4096mV internal reference.
+
-+ $ref: /schemas/types.yaml#/definitions/uint32
-+ enum: [2500, 4096]
-+ default: 4096
++ enum: [2500000, 4096000]
++ default: 4096000
+
+ required:
+ - reg
@@ Documentation/devicetree/bindings/iio/adc/adi,ad7949.yaml: examples:
+ vrefin-supply = <&vdd_supply>;
+
+ channel@0 {
-+ adi,internal-ref-mv = <4096>;
++ adi,internal-ref-microvolt = <4096000>;
+ reg = <0>;
+ };
+
+ channel@1 {
-+ adi,internal-ref-mv = <2500>;
++ adi,internal-ref-microvolt = <2500000>;
+ reg = <1>;
+ };
+
-: ------------ > 5: c48eb017058c iio: adc: ad7949: use devm managed functions
base-commit: 6cbb3aa0f9d5d23221df787cf36f74d3866fdb78
--
2.30.1.489.g328c10930387