From: David Lechner <[email protected]>
v3 changes:
* Added description of A0/A1 lines in DT bindings.
* Added power supply regulators to DT bindings.
* Dropped "staging: iio: Documentation: document IIO resolver AD2S1210
sysfs attributes" (these attributes are being removed instead).
* Dropped applied patches:
* "staging: iio: resolver: ad2s1210: fix ad2s1210_show_fault"
* "iio: adc: MCP3564: fix the static checker warning"
* Split "staging: iio: resolver: ad2s1210: fix probe" into multiple patches.
* Moved sorting imports to separate patch.
* Renamed fclkin to clkin_hz.
* Added __be16 sample field to state struct for reading raw samples.
* Split out new function ad2s1210_single_conversion() from
ad2s1210_read_raw().
* Split out new ad2s1210_get_hysteresis() and ad2s1210_set_hysteresis()
functions.
* Fixed multi-line comment style.
* Added notes about soft reset not resetting config registers.
* Made use of FIELD_PREP() macro.
* Added more explanation to regmap commit message.
* Removed datasheet names from channel specs.
* Replaced "staging: iio: resolver: ad2s1210: rename fexcit attribute"
with "staging: iio: resolver: ad2s1210: convert fexcit to channel
attribute".
* Replaced "staging: iio: resolver: ad2s1210: add phase_lock_range
attributes" with "staging: iio: resolver: ad2s1210: add phase lock
range support"
* Added additional patches to convert custom device attributes to event
attributes.
* Added patch to add channel label attributes.
v2 changes:
* Address initial device tree patch feedback
* Drop "iio: sysfs: add IIO_DEVICE_ATTR_NAMED_RW macro" (related cleanups
also dropped for now, will address in a future series if needed)
* Apply improvements as a series of patches to the staging driver. It is
not quite ready for the move out of staging patch yet.
This series has been tested on actual hardware using a EVAL-AD2S1210 evaluation
board. (Note: not all device tree features have been implemented in the driver
since the eval board doesn't support them out of the box. We plan to add them
later if needed.)
Most of the questions about dealing with faults from the v2 cover letter
have been addressed. There is still the question about what to do with
the current `fault` attribute (it is the only custom device attribute
remaining from the original staging driver). It was suggested to split it
out into multiple attributes in a subdirectory. Since we now have events
for all of the faults, I'm wondering if this is something that is still needed.
In the current implementation, it is possible to start listening to events,
clear the faults and then read a sample to trigger events for any current
faults so we have a way to get current faults already.
There is also the matter of clearing faults. Writing the excitation
frequency has a side-effect of clearing the faults, so we could use
that as the reset. Or we could change the current fault attribute to
write-only and rename it. Or is there a better way that I have overlooked?
Once this last issue is addressed, I think this driver will be ready
for consideration for moving out of staging.
---
David Lechner (27):
dt-bindings: iio: resolver: add devicetree bindings for ad2s1210
staging: iio: resolver: ad2s1210: fix use before initialization
staging: iio: resolver: ad2s1210: remove call to spi_setup()
staging: iio: resolver: ad2s1210: check return of ad2s1210_initial()
staging: iio: resolver: ad2s1210: remove spi_set_drvdata()
staging: iio: resolver: ad2s1210: sort imports
staging: iio: resolver: ad2s1210: always use 16-bit value for raw read
staging: iio: resolver: ad2s1210: implement IIO_CHAN_INFO_SCALE
staging: iio: resolver: ad2s1210: use devicetree to get CLKIN rate
staging: iio: resolver: ad2s1210: use regmap for config registers
staging: iio: resolver: ad2s1210: add debugfs reg access
staging: iio: resolver: ad2s1210: remove config attribute
staging: iio: resolver: ad2s1210: rework gpios
staging: iio: resolver: ad2s1210: implement hysteresis as channel attr
staging: iio: resolver: ad2s1210: refactor setting excitation frequency
staging: iio: resolver: ad2s1210: read excitation frequency from control register
staging: iio: resolver: ad2s1210: convert fexcit to channel attribute
staging: iio: resolver: ad2s1210: convert resolution to devicetree property
staging: iio: resolver: ad2s1210: add phase lock range support
staging: iio: resolver: ad2s1210: add triggered buffer support
staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs
staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr
staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr
staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
staging: iio: resolver: ad2s1210: implement fault events
staging: iio: resolver: ad2s1210: add label attribute support
.../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++
.../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 +
drivers/staging/iio/resolver/Kconfig | 1 +
drivers/staging/iio/resolver/ad2s1210.c | 1583 +++++++++++++++-----
4 files changed, 1391 insertions(+), 397 deletions(-)
---
base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec
change-id: 20230925-ad2s1210-mainline-2791ef75e386
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
There are quite a few imports and we will be adding more so it will
make it easier to read if they are sorted.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* This is a new patch split out from "staging: iio: resolver: ad2s1210:
use devicetree to get fclkin"
drivers/staging/iio/resolver/ad2s1210.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 28015322f562..832f86bf15e5 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -4,16 +4,16 @@
*
* Copyright (c) 2010-2010 Analog Devices Inc.
*/
-#include <linux/types.h>
-#include <linux/mutex.h>
+#include <linux/delay.h>
#include <linux/device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/of.h>
-#include <linux/spi/spi.h>
#include <linux/slab.h>
+#include <linux/spi/spi.h>
#include <linux/sysfs.h>
-#include <linux/delay.h>
-#include <linux/gpio/consumer.h>
-#include <linux/module.h>
+#include <linux/types.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
The AD2S1210 resolver has a hysteresis feature that can be used to
prevent flicker in the LSB of the position register. This can be either
enabled or disabled. Disabling hysteresis is useful for increasing
precision by oversampling.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* Refactored into more functions to reduce complexity of switch statements.
* Use early return instead of break in switch statements.
drivers/staging/iio/resolver/ad2s1210.c | 86 +++++++++++++++++++++++++++++++--
1 file changed, 83 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 0ec3598b600a..a82cb124a12f 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -78,7 +78,6 @@ struct ad2s1210_state {
/** The external oscillator frequency in Hz. */
unsigned long clkin_hz;
unsigned int fexcit;
- bool hysteresis;
u8 resolution;
/** For reading raw sample value via SPI. */
__be16 sample __aligned(IIO_DMA_MINALIGN);
@@ -430,6 +429,35 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
return ret;
}
+static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val)
+{
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_test_bits(st->regmap, AD2S1210_REG_CONTROL,
+ AD2S1210_ENABLE_HYSTERESIS);
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ *val = !!ret;
+ return IIO_VAL_INT;
+}
+
+static int ad2s1210_set_hysteresis(struct ad2s1210_state *st, int val)
+{
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
+ AD2S1210_ENABLE_HYSTERESIS,
+ val ? AD2S1210_ENABLE_HYSTERESIS : 0);
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
static const int ad2s1210_velocity_scale[] = {
17089132, /* 8.192MHz / (2*pi * 2500 / 2^15) */
42722830, /* 8.192MHz / (2*pi * 1000 / 2^15) */
@@ -462,7 +490,55 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_CHAN_INFO_HYSTERESIS:
+ switch (chan->type) {
+ case IIO_ANGL:
+ return ad2s1210_get_hysteresis(st, val);
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad2s1210_read_avail(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ const int **vals, int *type,
+ int *length, long mask)
+{
+ static const int hysteresis_available[] = { 0, 1 };
+
+ switch (mask) {
+ case IIO_CHAN_INFO_HYSTERESIS:
+ switch (chan->type) {
+ case IIO_ANGL:
+ *vals = hysteresis_available;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(hysteresis_available);
+ return IIO_AVAIL_LIST;
+ default:
+ return -EINVAL;
+ }
+ default:
+ return -EINVAL;
+ }
+}
+static int ad2s1210_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_HYSTERESIS:
+ switch (chan->type) {
+ case IIO_ANGL:
+ return ad2s1210_set_hysteresis(st, val);
+ default:
+ return -EINVAL;
+ }
default:
return -EINVAL;
}
@@ -503,7 +579,10 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.indexed = 1,
.channel = 0,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
- BIT(IIO_CHAN_INFO_SCALE),
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_HYSTERESIS),
+ .info_mask_separate_available =
+ BIT(IIO_CHAN_INFO_HYSTERESIS),
}, {
.type = IIO_ANGL_VEL,
.indexed = 1,
@@ -581,6 +660,8 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
static const struct iio_info ad2s1210_info = {
.read_raw = ad2s1210_read_raw,
+ .read_avail = ad2s1210_read_avail,
+ .write_raw = ad2s1210_write_raw,
.attrs = &ad2s1210_attribute_group,
.debugfs_reg_access = &ad2s1210_debugfs_reg_access,
};
@@ -696,7 +777,6 @@ static int ad2s1210_probe(struct spi_device *spi)
mutex_init(&st->lock);
st->sdev = spi;
- st->hysteresis = true;
st->resolution = 12;
st->fexcit = AD2S1210_DEF_EXCIT;
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This adds a check to the return value of ad2s1210_initial() since it
can fail. The call is also moved before devm_iio_device_register() so
that we don't have to unregister the device if it fails.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* This is a new patch split out from "staging: iio: resolver: ad2s1210:
fix probe"
drivers/staging/iio/resolver/ad2s1210.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 8fde08887f7f..b5e071d7c5fd 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -672,6 +672,10 @@ static int ad2s1210_probe(struct spi_device *spi)
if (ret < 0)
return ret;
+ ret = ad2s1210_initial(st);
+ if (ret < 0)
+ return ret;
+
indio_dev->info = &ad2s1210_info;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = ad2s1210_channels;
@@ -683,7 +687,6 @@ static int ad2s1210_probe(struct spi_device *spi)
return ret;
st->fclkin = spi->max_speed_hz;
- ad2s1210_initial(st);
return 0;
}
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This adds new DeviceTree bindings for the Analog Devices, Inc. AD2S1210
resolver-to-digital converter.
Co-developed-by: Apelete Seketeli <[email protected]>
Signed-off-by: Apelete Seketeli <[email protected]>
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* Expanded top-level description of A0/A1 lines.
* Added required voltage -supply properties. (I did not pick up Rob's
Reviewed-by since I wasn't sure if this was trivial enough.)
v2 changes:
* Add Co-developed-by:
* Remove extraneous quotes on strings
* Remove extraneous pipe on some multi-line descriptions
.../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++++++++++++++++++++
1 file changed, 177 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml b/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
new file mode 100644
index 000000000000..8980b3cd8337
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
@@ -0,0 +1,177 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/resolver/adi,ad2s1210.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD2S1210 Resolver-to-Digital Converter
+
+maintainers:
+ - Michael Hennerich <[email protected]>
+
+description: |
+ The AD2S1210 is a complete 10-bit to 16-bit resolution tracking
+ resolver-to-digital converter, integrating an on-board programmable
+ sinusoidal oscillator that provides sine wave excitation for
+ resolvers.
+
+ The AD2S1210 allows the user to read the angular position or the
+ angular velocity data directly from the parallel outputs or through
+ the serial interface.
+
+ The mode of operation of the communication channel (parallel or serial) is
+ selected by the A0 and A1 input pins. In normal mode, data is latched by
+ toggling the SAMPLE line and can then be read directly. In configuration mode,
+ data is read or written using a register access scheme (address byte with
+ read/write flag and data byte).
+
+ A1 A0 Result
+ 0 0 Normal mode - position output
+ 0 1 Normal mode - velocity output
+ 1 0 Reserved
+ 1 1 Configuration mode
+
+ In normal mode, the resolution of the digital output is selected using
+ the RES0 and RES1 input pins. In configuration mode, the resolution is
+ selected by setting the RES0 and RES1 bits in the control register.
+
+ RES1 RES0 Resolution (Bits)
+ 0 0 10
+ 0 1 12
+ 1 0 14
+ 1 1 16
+
+ Note on SPI connections: The CS line on the AD2S1210 should hard-wired to
+ logic low and the WR/FSYNC line on the AD2S1210 should be connected to the
+ SPI CSn output of the SPI controller.
+
+ Datasheet:
+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad2s1210.pdf
+
+properties:
+ compatible:
+ const: adi,ad2s1210
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 25000000
+
+ spi-cpha: true
+
+ avdd-supply:
+ description:
+ A 4.75 to 5.25 V regulator that powers the Analog Supply Voltage (AVDD)
+ pin.
+
+ dvdd-supply:
+ description:
+ A 4.75 to 5.25 V regulator that powers the Digital Supply Voltage (DVDD)
+ pin.
+
+ vdrive-supply:
+ description:
+ A 2.3 to 5.25 V regulator that powers the Logic Power Supply Input
+ (VDrive) pin.
+
+ clocks:
+ maxItems: 1
+ description: External oscillator clock (CLKIN).
+
+ reset-gpios:
+ description:
+ GPIO connected to the /RESET pin. As the line needs to be low for the
+ reset to be active, it should be configured as GPIO_ACTIVE_LOW.
+ maxItems: 1
+
+ sample-gpios:
+ description:
+ GPIO connected to the /SAMPLE pin. As the line needs to be low to trigger
+ a sample, it should be configured as GPIO_ACTIVE_LOW.
+ maxItems: 1
+
+ mode-gpios:
+ description:
+ GPIO lines connected to the A0 and A1 pins. These pins select the data
+ transfer mode.
+ minItems: 2
+ maxItems: 2
+
+ resolution-gpios:
+ description:
+ GPIO lines connected to the RES0 and RES1 pins. These pins select the
+ resolution of the digital output. If omitted, it is assumed that the
+ RES0 and RES1 pins are hard-wired to match the assigned-resolution-bits
+ property.
+ minItems: 2
+ maxItems: 2
+
+ fault-gpios:
+ description:
+ GPIO lines connected to the LOT and DOS pins. These pins combined indicate
+ the type of fault present, if any. As these pins a pulled low to indicate
+ a fault condition, they should be configured as GPIO_ACTIVE_LOW.
+ minItems: 2
+ maxItems: 2
+
+ adi,fixed-mode:
+ description:
+ This is used to indicate the selected mode if A0 and A1 are hard-wired
+ instead of connected to GPIOS (i.e. mode-gpios is omitted).
+ $ref: /schemas/types.yaml#/definitions/string
+ enum: [config, velocity, position]
+
+ assigned-resolution-bits:
+ description:
+ Resolution of the digital output required by the application. This
+ determines the precision of the angle and/or the maximum speed that can
+ be measured. If resolution-gpios is omitted, it is assumed that RES0 and
+ RES1 are hard-wired to match this value.
+ enum: [10, 12, 14, 16]
+
+required:
+ - compatible
+ - reg
+ - spi-cpha
+ - avdd-supply
+ - dvdd-supply
+ - vdrive-supply
+ - clocks
+ - sample-gpios
+ - assigned-resolution-bits
+
+oneOf:
+ - required:
+ - mode-gpios
+ - required:
+ - adi,fixed-mode
+
+allOf:
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ resolver@0 {
+ compatible = "adi,ad2s1210";
+ reg = <0>;
+ spi-max-frequency = <20000000>;
+ spi-cpha;
+ avdd-supply = <&avdd_regulator>;
+ dvdd-supply = <&dvdd_regulator>;
+ vdrive-supply = <&vdrive_regulator>;
+ clocks = <&ext_osc>;
+ sample-gpios = <&gpio0 90 GPIO_ACTIVE_LOW>;
+ mode-gpios = <&gpio0 86 0>, <&gpio0 87 0>;
+ resolution-gpios = <&gpio0 88 0>, <&gpio0 89 0>;
+ assigned-resolution-bits = <16>;
+ };
+ };
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This adds support for triggered buffers to the AD2S1210 resolver driver.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* Dropped setting datasheet_name of channels.
drivers/staging/iio/resolver/ad2s1210.c | 83 ++++++++++++++++++++++++++++++++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index bafc134eed97..c0bc9eac18e8 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -20,8 +20,11 @@
#include <linux/sysfs.h>
#include <linux/types.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
#define DRV_NAME "ad2s1210"
@@ -92,6 +95,12 @@ struct ad2s1210_state {
enum ad2s1210_resolution resolution;
/** For reading raw sample value via SPI. */
__be16 sample __aligned(IIO_DMA_MINALIGN);
+ /** Scan buffer */
+ struct {
+ __be16 chan[2];
+ /* Ensure timestamp is naturally aligned. */
+ s64 timestamp __aligned(8);
+ } scan;
/** SPI transmit buffer. */
u8 rx[2];
/** SPI receive buffer. */
@@ -617,6 +626,13 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.type = IIO_ANGL,
.indexed = 1,
.channel = 0,
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE) |
BIT(IIO_CHAN_INFO_HYSTERESIS),
@@ -626,9 +642,18 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.type = IIO_ANGL_VEL,
.indexed = 1,
.channel = 0,
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 16,
+ .storagebits = 16,
+ .endianness = IIO_BE,
+ },
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
- }, {
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(2),
+ {
/* used to configure phase lock range and get phase lock error */
.type = IIO_PHASE,
.indexed = 1,
@@ -756,6 +781,55 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
return ret;
}
+static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+ size_t chan = 0;
+ int ret;
+
+ mutex_lock(&st->lock);
+
+ memset(&st->scan, 0, sizeof(st->scan));
+ gpiod_set_value(st->sample_gpio, 1);
+
+ if (test_bit(0, indio_dev->active_scan_mask)) {
+ ret = ad2s1210_set_mode(st, MOD_POS);
+ if (ret < 0)
+ goto error_ret;
+
+ /* REVIST: we can read 3 bytes here and also get fault flags */
+ ret = spi_read(st->sdev, st->rx, 2);
+ if (ret < 0)
+ goto error_ret;
+
+ memcpy(&st->scan.chan[chan++], st->rx, 2);
+ }
+
+ if (test_bit(1, indio_dev->active_scan_mask)) {
+ ret = ad2s1210_set_mode(st, MOD_VEL);
+ if (ret < 0)
+ goto error_ret;
+
+ /* REVIST: we can read 3 bytes here and also get fault flags */
+ ret = spi_read(st->sdev, st->rx, 2);
+ if (ret < 0)
+ goto error_ret;
+
+ memcpy(&st->scan.chan[chan++], st->rx, 2);
+ }
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp);
+
+error_ret:
+ gpiod_set_value(st->sample_gpio, 0);
+ mutex_unlock(&st->lock);
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
static const struct iio_info ad2s1210_info = {
.event_attrs = &ad2s1210_event_attribute_group,
.read_raw = ad2s1210_read_raw,
@@ -944,6 +1018,13 @@ static int ad2s1210_probe(struct spi_device *spi)
indio_dev->num_channels = ARRAY_SIZE(ad2s1210_channels);
indio_dev->name = spi_get_device_id(spi)->name;
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ &iio_pollfunc_store_time,
+ &ad2s1210_trigger_handler, NULL);
+ if (ret < 0)
+ return dev_err_probe(&spi->dev, ret,
+ "iio triggered buffer setup failed\n");
+
return devm_iio_device_register(&spi->dev, indio_dev);
}
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This removes the config register sysfs attribute.
Writing to the config register directly can be dangerous and userspace
should not need to have to know the register layout. This register
can still be accessed though debugfs if needed.
We can add new attributes to set specific flags in the config register
in the future if needed (e.g. `enable_hysterisis` and
`phase_lock_range`).
Signed-off-by: David Lechner <[email protected]>
---
v3 changes: None
drivers/staging/iio/resolver/ad2s1210.c | 47 ---------------------------------
1 file changed, 47 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 31415fbb6384..2b9377447f6a 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -273,50 +273,6 @@ static ssize_t ad2s1210_store_fexcit(struct device *dev,
return ret < 0 ? ret : len;
}
-static ssize_t ad2s1210_show_control(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- unsigned int value;
- int ret;
-
- mutex_lock(&st->lock);
- ret = regmap_read(st->regmap, AD2S1210_REG_CONTROL, &value);
- mutex_unlock(&st->lock);
- return ret < 0 ? ret : sprintf(buf, "0x%x\n", value);
-}
-
-static ssize_t ad2s1210_store_control(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t len)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- unsigned char udata;
- unsigned char data;
- int ret;
-
- ret = kstrtou8(buf, 16, &udata);
- if (ret)
- return -EINVAL;
-
- mutex_lock(&st->lock);
- data = udata & ~AD2S1210_ADDRESS_DATA;
- ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
- if (ret < 0)
- goto error_ret;
-
- st->resolution =
- ad2s1210_resolution_value[data & AD2S1210_SET_RES];
- ad2s1210_set_resolution_pin(st);
- ret = len;
- st->hysteresis = !!(data & AD2S1210_ENABLE_HYSTERESIS);
-
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
-}
-
static ssize_t ad2s1210_show_resolution(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -523,8 +479,6 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
static IIO_DEVICE_ATTR(fexcit, 0644,
ad2s1210_show_fexcit, ad2s1210_store_fexcit, 0);
-static IIO_DEVICE_ATTR(control, 0644,
- ad2s1210_show_control, ad2s1210_store_control, 0);
static IIO_DEVICE_ATTR(bits, 0644,
ad2s1210_show_resolution, ad2s1210_store_resolution, 0);
static IIO_DEVICE_ATTR(fault, 0644,
@@ -570,7 +524,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
static struct attribute *ad2s1210_attributes[] = {
&iio_dev_attr_fexcit.dev_attr.attr,
- &iio_dev_attr_control.dev_attr.attr,
&iio_dev_attr_bits.dev_attr.attr,
&iio_dev_attr_fault.dev_attr.attr,
&iio_dev_attr_los_thrd.dev_attr.attr,
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
The AD2S1210 has a programmable threshold for the degradation of signal
(DOS) mismatch fault. This fault is triggered when the difference in
amplitude between the sine and cosine inputs exceeds the threshold.
The DOS reset min/max registers on the chip provide initial values
for internal tracking of the min/max of the monitor signal after the
fault register is cleared.
This patch converts the custom device DOS reset min/max threshold
attributes custom event attributes on the monitor signal channel.
The attributes now use millivolts instead of the raw register value in
accordance with the IIO ABI.
Emitting the event will be implemented in a later patch.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes: This is a new patch in v3
.../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 ++++++
drivers/staging/iio/resolver/ad2s1210.c | 99 ++++++++++++----------
2 files changed, 82 insertions(+), 44 deletions(-)
diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
new file mode 100644
index 000000000000..ea75881b0c77
--- /dev/null
+++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
@@ -0,0 +1,27 @@
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max
+KernelVersion: 6.7
+Contact: [email protected]
+Description:
+ Reading returns the current Degradation of Signal Reset Maximum
+ Threshold value in millivolts. Writing sets the value.
+
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max_available
+KernelVersion: 6.7
+Contact: [email protected]
+Description:
+ Reading returns the allowable voltage range for
+ in_altvoltage0-altvoltage1_thresh_rising_reset_max.
+
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min
+KernelVersion: 6.7
+Contact: [email protected]
+Description:
+ Reading returns the current Degradation of Signal Reset Minimum
+ Threshold value in millivolts. Writing sets the value.
+
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min_available
+KernelVersion: 6.7
+Contact: [email protected]
+Description:
+ Reading returns the allowable voltage range for
+ in_altvoltage0-altvoltage1_thresh_rising_reset_min.
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index aa14edbe8a77..e1c95ec73545 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -283,41 +283,6 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
return ret < 0 ? ret : len;
}
-static ssize_t ad2s1210_show_reg(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
- unsigned int value;
- int ret;
-
- mutex_lock(&st->lock);
- ret = regmap_read(st->regmap, iattr->address, &value);
- mutex_unlock(&st->lock);
-
- return ret < 0 ? ret : sprintf(buf, "%d\n", value);
-}
-
-static ssize_t ad2s1210_store_reg(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t len)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- unsigned char data;
- int ret;
- struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
-
- ret = kstrtou8(buf, 10, &data);
- if (ret)
- return -EINVAL;
-
- mutex_lock(&st->lock);
- ret = regmap_write(st->regmap, iattr->address, data);
- mutex_unlock(&st->lock);
- return ret < 0 ? ret : len;
-}
-
static int ad2s1210_single_conversion(struct ad2s1210_state *st,
struct iio_chan_spec const *chan,
int *val)
@@ -743,13 +708,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
static IIO_DEVICE_ATTR(fault, 0644,
ad2s1210_show_fault, ad2s1210_clear_fault, 0);
-static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
- ad2s1210_show_reg, ad2s1210_store_reg,
- AD2S1210_REG_DOS_RST_MAX_THRD);
-static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
- ad2s1210_show_reg, ad2s1210_store_reg,
- AD2S1210_REG_DOS_RST_MIN_THRD);
-
static const struct iio_event_spec ad2s1210_position_event_spec[] = {
{
/* Tracking error exceeds LOT threshold fault. */
@@ -867,8 +825,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
static struct attribute *ad2s1210_attributes[] = {
&iio_dev_attr_fault.dev_attr.attr,
- &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
- &iio_dev_attr_dos_rst_min_thrd.dev_attr.attr,
NULL,
};
@@ -876,6 +832,49 @@ static const struct attribute_group ad2s1210_attribute_group = {
.attrs = ad2s1210_attributes,
};
+static ssize_t event_attr_voltage_reg_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+ struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
+ unsigned int value;
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_read(st->regmap, iattr->address, &value);
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ return sprintf(buf, "%d\n", value * THRESHOLD_MILLIVOLT_PER_LSB);
+}
+
+static ssize_t event_attr_voltage_reg_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t len)
+{
+ struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+ struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
+ u16 data;
+ int ret;
+
+ ret = kstrtou16(buf, 10, &data);
+ if (ret)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+ ret = regmap_write(st->regmap, iattr->address,
+ data / THRESHOLD_MILLIVOLT_PER_LSB);
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ return len;
+}
+
static ssize_t
in_angl1_thresh_rising_value_available_show(struct device *dev,
struct device_attribute *attr,
@@ -906,6 +905,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR);
IIO_CONST_ATTR(in_altvoltage0_mag_value_available, THRESHOLD_RANGE_STR);
+IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_max, 0644,
+ event_attr_voltage_reg_show, event_attr_voltage_reg_store,
+ AD2S1210_REG_DOS_RST_MAX_THRD);
+IIO_CONST_ATTR(in_altvoltage0_mag_reset_max_available, THRESHOLD_RANGE_STR);
+IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_min, 0644,
+ event_attr_voltage_reg_show, event_attr_voltage_reg_store,
+ AD2S1210_REG_DOS_RST_MIN_THRD);
+IIO_CONST_ATTR(in_altvoltage0_mag_reset_min_available, THRESHOLD_RANGE_STR);
IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
@@ -914,6 +921,10 @@ static struct attribute *ad2s1210_event_attributes[] = {
&iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
&iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr,
&iio_const_attr_in_altvoltage0_mag_value_available.dev_attr.attr,
+ &iio_dev_attr_in_altvoltage0_mag_reset_max.dev_attr.attr,
+ &iio_const_attr_in_altvoltage0_mag_reset_max_available.dev_attr.attr,
+ &iio_dev_attr_in_altvoltage0_mag_reset_min.dev_attr.attr,
+ &iio_const_attr_in_altvoltage0_mag_reset_min_available.dev_attr.attr,
&iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
&iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
NULL,
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This adds an implementation of IIO_CHAN_INFO_SCALE to the ad2s1210
resolver driver. This allows userspace to get the scale factor for the
raw values.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* Split ad2s1210_read_raw() into two functions to reduce complexity.
* Use early return instead of break in switch statements.
drivers/staging/iio/resolver/ad2s1210.c | 53 ++++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index f9774dff2df4..a710598a64f0 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -461,13 +461,10 @@ static ssize_t ad2s1210_store_reg(struct device *dev,
return ret < 0 ? ret : len;
}
-static int ad2s1210_read_raw(struct iio_dev *indio_dev,
- struct iio_chan_spec const *chan,
- int *val,
- int *val2,
- long m)
+static int ad2s1210_single_conversion(struct ad2s1210_state *st,
+ struct iio_chan_spec const *chan,
+ int *val)
{
- struct ad2s1210_state *st = iio_priv(indio_dev);
int ret = 0;
mutex_lock(&st->lock);
@@ -514,6 +511,44 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
return ret;
}
+static const int ad2s1210_velocity_scale[] = {
+ 17089132, /* 8.192MHz / (2*pi * 2500 / 2^15) */
+ 42722830, /* 8.192MHz / (2*pi * 1000 / 2^15) */
+ 85445659, /* 8.192MHz / (2*pi * 500 / 2^15) */
+ 341782638, /* 8.192MHz / (2*pi * 125 / 2^15) */
+};
+
+static int ad2s1210_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val,
+ int *val2,
+ long mask)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ return ad2s1210_single_conversion(st, chan, val);
+ case IIO_CHAN_INFO_SCALE:
+ switch (chan->type) {
+ case IIO_ANGL:
+ /* approx 0.3 arc min converted to radians */
+ *val = 0;
+ *val2 = 95874;
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_ANGL_VEL:
+ *val = st->fclkin;
+ *val2 = ad2s1210_velocity_scale[st->resolution];
+ return IIO_VAL_FRACTIONAL;
+ default:
+ return -EINVAL;
+ }
+
+ default:
+ return -EINVAL;
+ }
+}
+
static IIO_DEVICE_ATTR(fclkin, 0644,
ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0);
static IIO_DEVICE_ATTR(fexcit, 0644,
@@ -552,12 +587,14 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.type = IIO_ANGL,
.indexed = 1,
.channel = 0,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
}, {
.type = IIO_ANGL_VEL,
.indexed = 1,
.channel = 0,
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE),
}
};
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
The ad2s1210 resolver driver has quite a few channels, mostly for
internal signals for event support. This makes it difficult to know
which channel is which. This patch adds a label attribute to the
channels to make it easier to identify them.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes: This is a new patch in v3
drivers/staging/iio/resolver/ad2s1210.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index dc3cc3ab855e..a187fa07d315 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -1106,6 +1106,34 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
return ret;
}
+static int ad2s1210_read_label(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ char *label)
+{
+ if (chan->type == IIO_ANGL) {
+ if (chan->channel == 0)
+ return sprintf(label, "position\n");
+ if (chan->channel == 1)
+ return sprintf(label, "tracking error\n");
+ }
+ if (chan->type == IIO_ANGL_VEL)
+ return sprintf(label, "velocity\n");
+ if (chan->type == IIO_PHASE)
+ return sprintf(label, "synthetic reference\n");
+ if (chan->type == IIO_ALTVOLTAGE) {
+ if (chan->output)
+ return sprintf(label, "excitation\n");
+ if (chan->channel == 0)
+ return sprintf(label, "monitor signal\n");
+ if (chan->channel == 1 && chan->channel2 == IIO_MOD_X)
+ return sprintf(label, "cosine\n");
+ if (chan->channel == 1 && chan->channel2 == IIO_MOD_Y)
+ return sprintf(label, "sine\n");
+ }
+
+ return -EINVAL;
+}
+
static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan,
enum iio_event_type type,
@@ -1256,6 +1284,7 @@ static const struct iio_info ad2s1210_info = {
.read_raw = ad2s1210_read_raw,
.read_avail = ad2s1210_read_avail,
.write_raw = ad2s1210_write_raw,
+ .read_label = ad2s1210_read_label,
.attrs = &ad2s1210_attribute_group,
.read_event_value = ad2s1210_read_event_value,
.write_event_value = ad2s1210_write_event_value,
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
When reading the position and velocity on the AD2S1210, there is also a
3rd byte following the two data bytes that contains the fault flag bits.
This patch adds support for reading this byte and generating events when
faults occur.
The faults are mapped to various channels and event types in order to
have a unique event for each fault.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes: This is a new patch in v3
drivers/staging/iio/resolver/ad2s1210.c | 175 +++++++++++++++++++++++++++++---
1 file changed, 161 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index e1c95ec73545..dc3cc3ab855e 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -21,6 +21,7 @@
#include <linux/types.h>
#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/iio/trigger_consumer.h>
@@ -35,6 +36,16 @@
#define AD2S1210_SET_ENRES GENMASK(3, 2)
#define AD2S1210_SET_RES GENMASK(1, 0)
+/* fault register flags */
+#define AD2S1210_FAULT_CLIP BIT(7)
+#define AD2S1210_FAULT_LOS BIT(6)
+#define AD2S1210_FAULT_DOS_OVR BIT(5)
+#define AD2S1210_FAULT_DOS_MIS BIT(4)
+#define AD2S1210_FAULT_LOT BIT(3)
+#define AD2S1210_FAULT_VELOCITY BIT(2)
+#define AD2S1210_FAULT_PHASE BIT(1)
+#define AD2S1210_FAULT_CONFIG_PARITY BIT(0)
+
#define AD2S1210_REG_POSITION_MSB 0x80
#define AD2S1210_REG_POSITION_LSB 0x81
#define AD2S1210_REG_VELOCITY_MSB 0x82
@@ -71,6 +82,8 @@
/* max voltage for threshold registers is 0x7F * 38 mV */
#define THRESHOLD_RANGE_STR "[0 38 4826]"
+#define FAULT_ONESHOT(bit, new, old) (new & bit && !(old & bit))
+
enum ad2s1210_mode {
MOD_POS = 0b00,
MOD_VEL = 0b01,
@@ -98,8 +111,13 @@ struct ad2s1210_state {
unsigned long clkin_hz;
/** The selected resolution */
enum ad2s1210_resolution resolution;
+ /** Copy of fault register from the previous read. */
+ u8 prev_fault_flags;
/** For reading raw sample value via SPI. */
- __be16 sample __aligned(IIO_DMA_MINALIGN);
+ struct {
+ __be16 raw;
+ u8 fault;
+ } sample __aligned(IIO_DMA_MINALIGN);;
/** Scan buffer */
struct {
__be16 chan[2];
@@ -158,7 +176,15 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
if (ret < 0)
return ret;
- return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
+ ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
+ if (ret < 0)
+ return ret;
+
+ /* soft reset also clears the fault register */
+ if (reg == AD2S1210_REG_SOFT_RESET)
+ st->prev_fault_flags = 0;
+
+ return 0;
}
/*
@@ -200,6 +226,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
if (ret < 0)
return ret;
+ /* reading the fault register also clears it */
+ if (reg == AD2S1210_REG_FAULT)
+ st->prev_fault_flags = 0;
+
/*
* If the D7 bit is set on any read/write register, it indicates a
* parity error. The fault register is read-only and the D7 bit means
@@ -283,14 +313,92 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
return ret < 0 ? ret : len;
}
-static int ad2s1210_single_conversion(struct ad2s1210_state *st,
+static void ad2s1210_push_events(struct iio_dev *indio_dev,
+ u8 flags, s64 timestamp)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+
+ /* Sine/cosine inputs clipped */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_CLIP, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_MOD_EVENT_CODE(IIO_ALTVOLTAGE, 1,
+ IIO_MOD_X_OR_Y,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_NONE),
+ timestamp);
+
+ /* Sine/cosine inputs below LOS threshold */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_LOS, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_FALLING),
+ timestamp);
+
+ /* Sine/cosine inputs exceed DOS overrange threshold */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_OVR, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING),
+ timestamp);
+
+ /* Sine/cosine inputs exceed DOS mismatch threshold */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_MIS, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_NONE),
+ timestamp);
+
+ /* Tracking error exceeds LOT threshold */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_LOT, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ANGL, 1,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING),
+ timestamp);
+
+ /* Velocity exceeds maximum tracking rate */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_VELOCITY, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ANGL_VEL, 0,
+ IIO_EV_TYPE_THRESH,
+ IIO_EV_DIR_RISING),
+ timestamp);
+
+ /* Phase error exceeds phase lock range */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_PHASE, flags, st->prev_fault_flags))
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_PHASE, 0,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_NONE),
+ timestamp);
+
+ /* Configuration parity error */
+ if (FAULT_ONESHOT(AD2S1210_FAULT_CONFIG_PARITY, flags,
+ st->prev_fault_flags))
+ /*
+ * Userspace should also get notified of this via error return
+ * when trying to write to any attribute that writes a register.
+ */
+ dev_err_ratelimited(&indio_dev->dev,
+ "Configuration parity error\n");
+
+ st->prev_fault_flags = flags;
+}
+
+static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan,
int *val)
{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+ s64 timestamp;
int ret;
mutex_lock(&st->lock);
gpiod_set_value(st->sample_gpio, 1);
+ timestamp = iio_get_time_ns(indio_dev);
/* delay (6 * tck + 20) nano seconds */
udelay(1);
@@ -307,17 +415,17 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
}
if (ret < 0)
goto error_ret;
- ret = spi_read(st->sdev, &st->sample, 2);
+ ret = spi_read(st->sdev, &st->sample, 3);
if (ret < 0)
goto error_ret;
switch (chan->type) {
case IIO_ANGL:
- *val = be16_to_cpu(st->sample);
+ *val = be16_to_cpu(st->sample.raw);
ret = IIO_VAL_INT;
break;
case IIO_ANGL_VEL:
- *val = (s16)be16_to_cpu(st->sample);
+ *val = (s16)be16_to_cpu(st->sample.raw);
ret = IIO_VAL_INT;
break;
default:
@@ -325,6 +433,8 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
break;
}
+ ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
+
error_ret:
gpiod_set_value(st->sample_gpio, 0);
/* delay (2 * tck + 20) nano seconds */
@@ -608,7 +718,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
- return ad2s1210_single_conversion(st, chan, val);
+ return ad2s1210_single_conversion(indio_dev, chan, val);
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_ANGL:
@@ -721,6 +831,14 @@ static const struct iio_event_spec ad2s1210_position_event_spec[] = {
},
};
+static const struct iio_event_spec ad2s1210_velocity_event_spec[] = {
+ {
+ /* Velocity exceeds maximum tracking rate fault. */
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ },
+};
+
static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
{
/* Phase error fault. */
@@ -754,6 +872,14 @@ static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
},
};
+static const struct iio_event_spec ad2s1210_sin_cos_event_spec[] = {
+ {
+ /* Sine/cosine clipping fault. */
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_NONE,
+ },
+};
+
static const struct iio_chan_spec ad2s1210_channels[] = {
{
.type = IIO_ANGL,
@@ -784,6 +910,8 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
},
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
+ .event_spec = ad2s1210_velocity_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_velocity_event_spec),
},
IIO_CHAN_SOFT_TIMESTAMP(2),
{
@@ -820,6 +948,26 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.scan_index = -1,
.event_spec = ad2s1210_monitor_signal_event_spec,
.num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
+ }, {
+ /* sine input */
+ .type = IIO_ALTVOLTAGE,
+ .indexed = 1,
+ .channel = 1,
+ .modified = 1,
+ .channel2 = IIO_MOD_Y,
+ .scan_index = -1,
+ .event_spec = ad2s1210_sin_cos_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec),
+ }, {
+ /* cosine input */
+ .type = IIO_ALTVOLTAGE,
+ .indexed = 1,
+ .channel = 1,
+ .modified = 1,
+ .channel2 = IIO_MOD_X,
+ .scan_index = -1,
+ .event_spec = ad2s1210_sin_cos_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec),
},
};
@@ -936,7 +1084,7 @@ static const struct attribute_group ad2s1210_event_attribute_group = {
static int ad2s1210_initial(struct ad2s1210_state *st)
{
- unsigned char data;
+ unsigned int data;
int ret;
mutex_lock(&st->lock);
@@ -1073,12 +1221,11 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
if (ret < 0)
goto error_ret;
- /* REVIST: we can read 3 bytes here and also get fault flags */
- ret = spi_read(st->sdev, st->rx, 2);
+ ret = spi_read(st->sdev, &st->sample, 3);
if (ret < 0)
goto error_ret;
- memcpy(&st->scan.chan[chan++], st->rx, 2);
+ memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
}
if (test_bit(1, indio_dev->active_scan_mask)) {
@@ -1086,14 +1233,14 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
if (ret < 0)
goto error_ret;
- /* REVIST: we can read 3 bytes here and also get fault flags */
- ret = spi_read(st->sdev, st->rx, 2);
+ ret = spi_read(st->sdev, &st->sample, 3);
if (ret < 0)
goto error_ret;
- memcpy(&st->scan.chan[chan++], st->rx, 2);
+ memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
}
+ ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp);
error_ret:
--
2.42.0
On 9/29/23 12:23 PM, David Lechner wrote:
> From: David Lechner <[email protected]>
>
> v3 changes:
>
> * Added description of A0/A1 lines in DT bindings.
> * Added power supply regulators to DT bindings.
> * Dropped "staging: iio: Documentation: document IIO resolver AD2S1210
> sysfs attributes" (these attributes are being removed instead).
> * Dropped applied patches:
> * "staging: iio: resolver: ad2s1210: fix ad2s1210_show_fault"
> * "iio: adc: MCP3564: fix the static checker warning"
> * Split "staging: iio: resolver: ad2s1210: fix probe" into multiple patches.
> * Moved sorting imports to separate patch.
> * Renamed fclkin to clkin_hz.
> * Added __be16 sample field to state struct for reading raw samples.
> * Split out new function ad2s1210_single_conversion() from
> ad2s1210_read_raw().
> * Split out new ad2s1210_get_hysteresis() and ad2s1210_set_hysteresis()
> functions.
> * Fixed multi-line comment style.
> * Added notes about soft reset not resetting config registers.
> * Made use of FIELD_PREP() macro.
> * Added more explanation to regmap commit message.
> * Removed datasheet names from channel specs.
> * Replaced "staging: iio: resolver: ad2s1210: rename fexcit attribute"
> with "staging: iio: resolver: ad2s1210: convert fexcit to channel
> attribute".
> * Replaced "staging: iio: resolver: ad2s1210: add phase_lock_range
> attributes" with "staging: iio: resolver: ad2s1210: add phase lock
> range support"
> * Added additional patches to convert custom device attributes to event
> attributes.
> * Added patch to add channel label attributes.
>
> v2 changes:
> * Address initial device tree patch feedback
> * Drop "iio: sysfs: add IIO_DEVICE_ATTR_NAMED_RW macro" (related cleanups
> also dropped for now, will address in a future series if needed)
> * Apply improvements as a series of patches to the staging driver. It is
> not quite ready for the move out of staging patch yet.
>
> This series has been tested on actual hardware using a EVAL-AD2S1210 evaluation
> board. (Note: not all device tree features have been implemented in the driver
> since the eval board doesn't support them out of the box. We plan to add them
> later if needed.)
>
> Most of the questions about dealing with faults from the v2 cover letter
> have been addressed. There is still the question about what to do with
> the current `fault` attribute (it is the only custom device attribute
> remaining from the original staging driver). It was suggested to split it
> out into multiple attributes in a subdirectory. Since we now have events
> for all of the faults, I'm wondering if this is something that is still needed.
> In the current implementation, it is possible to start listening to events,
> clear the faults and then read a sample to trigger events for any current
> faults so we have a way to get current faults already.
>
> There is also the matter of clearing faults. Writing the excitation
> frequency has a side-effect of clearing the faults, so we could use
> that as the reset. Or we could change the current fault attribute to
> write-only and rename it. Or is there a better way that I have overlooked?
>
> Once this last issue is addressed, I think this driver will be ready
> for consideration for moving out of staging.
> ---
> David Lechner (27):
> dt-bindings: iio: resolver: add devicetree bindings for ad2s1210
> staging: iio: resolver: ad2s1210: fix use before initialization
> staging: iio: resolver: ad2s1210: remove call to spi_setup()
> staging: iio: resolver: ad2s1210: check return of ad2s1210_initial()
> staging: iio: resolver: ad2s1210: remove spi_set_drvdata()
> staging: iio: resolver: ad2s1210: sort imports
> staging: iio: resolver: ad2s1210: always use 16-bit value for raw read
> staging: iio: resolver: ad2s1210: implement IIO_CHAN_INFO_SCALE
> staging: iio: resolver: ad2s1210: use devicetree to get CLKIN rate
> staging: iio: resolver: ad2s1210: use regmap for config registers
> staging: iio: resolver: ad2s1210: add debugfs reg access
> staging: iio: resolver: ad2s1210: remove config attribute
> staging: iio: resolver: ad2s1210: rework gpios
> staging: iio: resolver: ad2s1210: implement hysteresis as channel attr
> staging: iio: resolver: ad2s1210: refactor setting excitation frequency
> staging: iio: resolver: ad2s1210: read excitation frequency from control register
> staging: iio: resolver: ad2s1210: convert fexcit to channel attribute
> staging: iio: resolver: ad2s1210: convert resolution to devicetree property
> staging: iio: resolver: ad2s1210: add phase lock range support
> staging: iio: resolver: ad2s1210: add triggered buffer support
> staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs
> staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
> staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr
> staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr
> staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
> staging: iio: resolver: ad2s1210: implement fault events
> staging: iio: resolver: ad2s1210: add label attribute support
>
> .../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++
> .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 +
> drivers/staging/iio/resolver/Kconfig | 1 +
> drivers/staging/iio/resolver/ad2s1210.c | 1583 +++++++++++++++-----
> 4 files changed, 1391 insertions(+), 397 deletions(-)
> ---
> base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec
> change-id: 20230925-ad2s1210-mainline-2791ef75e386
>
In the end, this is what sysfs looks like:
root@analog:~# tree /sys/bus/iio/devices/iio\:device1/
/sys/bus/iio/devices/iio:device1/
├── buffer
│ ├── data_available
│ ├── direction
│ ├── enable
│ ├── length
│ └── watermark
├── buffer0
│ ├── data_available
│ ├── direction
│ ├── enable
│ ├── in_angl0_en
│ ├── in_angl0_index
│ ├── in_angl0_type
│ ├── in_anglvel0_en
│ ├── in_anglvel0_index
│ ├── in_anglvel0_type
│ ├── in_timestamp_en
│ ├── in_timestamp_index
│ ├── in_timestamp_type
│ ├── length
│ └── watermark
├── current_timestamp_clock
├── dev
├── events
│ ├── in_altvoltage0_mag_reset_max
│ ├── in_altvoltage0_mag_reset_max_available
│ ├── in_altvoltage0_mag_reset_min
│ ├── in_altvoltage0_mag_reset_min_available
│ ├── in_altvoltage0_mag_value
│ ├── in_altvoltage0_mag_value_available
│ ├── in_altvoltage0_thresh_falling_value
│ ├── in_altvoltage0_thresh_falling_value_available
│ ├── in_altvoltage0_thresh_rising_value
│ ├── in_altvoltage0_thresh_rising_value_available
│ ├── in_angl1_thresh_rising_hysteresis
│ ├── in_angl1_thresh_rising_hysteresis_available
│ ├── in_angl1_thresh_rising_value
│ ├── in_angl1_thresh_rising_value_available
│ ├── in_phase0_mag_value
│ └── in_phase0_mag_value_available
├── fault
├── in_altvoltage0_label
├── in_altvoltage1_x_label
├── in_altvoltage1_y_label
├── in_angl0_hysteresis
├── in_angl0_hysteresis_available
├── in_angl0_label
├── in_angl0_raw
├── in_angl0_scale
├── in_angl1_label
├── in_anglvel0_label
├── in_anglvel0_raw
├── in_anglvel0_scale
├── in_phase0_label
├── name
├── of_node -> ../../../../../../../../firmware/devicetree/base/axi/spi@e0006000/ad2s1210@0
├── out_altvoltage0_frequency
├── out_altvoltage0_frequency_available
├── out_altvoltage0_label
├── power
│ ├── autosuspend_delay_ms
│ ├── control
│ ├── runtime_active_time
│ ├── runtime_status
│ └── runtime_suspended_time
├── scan_elements
│ ├── in_angl0_en
│ ├── in_angl0_index
│ ├── in_angl0_type
│ ├── in_anglvel0_en
│ ├── in_anglvel0_index
│ ├── in_anglvel0_type
│ ├── in_timestamp_en
│ ├── in_timestamp_index
│ └── in_timestamp_type
├── subsystem -> ../../../../../../../../bus/iio
├── trigger
│ └── current_trigger
├── uevent
└── waiting_for_supplier
8 directories, 72 files
And this is what the output of iio_info looks like:
(note: iio_info does not currently support events so those
attributes are not visible here)
iio:device1: ad2s1210 (buffer capable)
9 channels found:
angl0: (input, index: 0, format: be:U16/16>>0)
5 channel-specific attributes found:
attr 0: hysteresis value: 1
attr 1: hysteresis_available value: 0 1
attr 2: label value: position
attr 3: raw value: 12578
attr 4: scale value: 0.000095874
anglvel0: (input, index: 1, format: be:S16/16>>0)
3 channel-specific attributes found:
attr 0: label value: velocity
attr 1: raw value: 5
attr 2: scale value: 0.023968449
timestamp: (input, index: 2, format: le:S64/64>>0)
phase0: (input)
1 channel-specific attributes found:
attr 0: label value: synthetic reference
altvoltage0: (output)
3 channel-specific attributes found:
attr 0: frequency value: 10000
attr 1: frequency_available value: [2000 250 20000]
attr 2: label value: excitation
angl1: (input)
1 channel-specific attributes found:
attr 0: label value: tracking error
altvoltage1_y: (input)
1 channel-specific attributes found:
attr 0: label value: sine
altvoltage0: (input)
1 channel-specific attributes found:
attr 0: label value: monitor signal
altvoltage1_x: (input)
1 channel-specific attributes found:
attr 0: label value: cosine
3 device-specific attributes found:
attr 0: current_timestamp_clock value: realtime
attr 1: fault value: 0x00
attr 2: waiting_for_supplier value: 0
2 buffer-specific attributes found:
attr 0: data_available value: 0
attr 1: direction value: in
1 debug attributes found:
debug attr 0: direct_reg_access ERROR: Input/output error (5)
Current trigger: trigger0(test)
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-26-fa4364281745%40baylibre.com
patch subject: [PATCH v3 26/27] staging: iio: resolver: ad2s1210: implement fault events
config: i386-randconfig-003-20230930 (https://download.01.org/0day-ci/archive/20230930/[email protected]/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230930/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
drivers/staging/iio/resolver/ad2s1210.c:1452:34: warning: 'ad2s1210_of_match' defined but not used [-Wunused-const-variable=]
1452 | static const struct of_device_id ad2s1210_of_match[] = {
| ^~~~~~~~~~~~~~~~~
drivers/staging/iio/resolver/ad2s1210.c: In function 'ad2s1210_read_raw':
>> drivers/staging/iio/resolver/ad2s1210.c:436:40: warning: array subscript 2 is above array bounds of 'u8[2]' {aka 'unsigned char[2]'} [-Warray-bounds]
436 | ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
| ~~~~~~^~~
vim +436 drivers/staging/iio/resolver/ad2s1210.c
390
391 static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
392 struct iio_chan_spec const *chan,
393 int *val)
394 {
395 struct ad2s1210_state *st = iio_priv(indio_dev);
396 s64 timestamp;
397 int ret;
398
399 mutex_lock(&st->lock);
400 gpiod_set_value(st->sample_gpio, 1);
401 timestamp = iio_get_time_ns(indio_dev);
402 /* delay (6 * tck + 20) nano seconds */
403 udelay(1);
404
405 switch (chan->type) {
406 case IIO_ANGL:
407 ret = ad2s1210_set_mode(st, MOD_POS);
408 break;
409 case IIO_ANGL_VEL:
410 ret = ad2s1210_set_mode(st, MOD_VEL);
411 break;
412 default:
413 ret = -EINVAL;
414 break;
415 }
416 if (ret < 0)
417 goto error_ret;
418 ret = spi_read(st->sdev, &st->sample, 3);
419 if (ret < 0)
420 goto error_ret;
421
422 switch (chan->type) {
423 case IIO_ANGL:
424 *val = be16_to_cpu(st->sample.raw);
425 ret = IIO_VAL_INT;
426 break;
427 case IIO_ANGL_VEL:
428 *val = (s16)be16_to_cpu(st->sample.raw);
429 ret = IIO_VAL_INT;
430 break;
431 default:
432 ret = -EINVAL;
433 break;
434 }
435
> 436 ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
437
438 error_ret:
439 gpiod_set_value(st->sample_gpio, 0);
440 /* delay (2 * tck + 20) nano seconds */
441 udelay(1);
442 mutex_unlock(&st->lock);
443 return ret;
444 }
445
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, 29 Sep 2023 12:23:09 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> This adds a check to the return value of ad2s1210_initial() since it
> can fail. The call is also moved before devm_iio_device_register() so
> that we don't have to unregister the device if it fails.
>
> Signed-off-by: David Lechner <[email protected]>
Applied
> ---
>
> v3 changes:
> * This is a new patch split out from "staging: iio: resolver: ad2s1210:
> fix probe"
>
> drivers/staging/iio/resolver/ad2s1210.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 8fde08887f7f..b5e071d7c5fd 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -672,6 +672,10 @@ static int ad2s1210_probe(struct spi_device *spi)
> if (ret < 0)
> return ret;
>
> + ret = ad2s1210_initial(st);
> + if (ret < 0)
> + return ret;
> +
> indio_dev->info = &ad2s1210_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = ad2s1210_channels;
> @@ -683,7 +687,6 @@ static int ad2s1210_probe(struct spi_device *spi)
> return ret;
>
> st->fclkin = spi->max_speed_hz;
> - ad2s1210_initial(st);
>
> return 0;
> }
>
On Fri, 29 Sep 2023 12:23:06 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> This adds new DeviceTree bindings for the Analog Devices, Inc. AD2S1210
> resolver-to-digital converter.
>
> Co-developed-by: Apelete Seketeli <[email protected]>
> Signed-off-by: Apelete Seketeli <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
Michael, ideally I'd like your ack on this given it volunteers you as
maintainer. If I don't hear I'm fine with leaving Michael listed
because he's in MAINTAINERS anyway covering these bindings via
a wild card entry:
ANALOG DEVICES INC IIO DRIVERS
M: Lars-Peter Clausen <[email protected]>
M: Michael Hennerich <[email protected]>
...
F: Documentation/devicetree/bindings/iio/*/adi,*
So any queries should hit Michael anyway.
LGTM but I'll also want the dt binding maintainers input before picking this
up.
Jonathan
> ---
>
> v3 changes:
> * Expanded top-level description of A0/A1 lines.
> * Added required voltage -supply properties. (I did not pick up Rob's
> Reviewed-by since I wasn't sure if this was trivial enough.)
>
> v2 changes:
> * Add Co-developed-by:
> * Remove extraneous quotes on strings
> * Remove extraneous pipe on some multi-line descriptions
>
> .../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++++++++++++++++++++
> 1 file changed, 177 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml b/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
> new file mode 100644
> index 000000000000..8980b3cd8337
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
> @@ -0,0 +1,177 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/resolver/adi,ad2s1210.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD2S1210 Resolver-to-Digital Converter
> +
> +maintainers:
> + - Michael Hennerich <[email protected]>
> +
> +description: |
> + The AD2S1210 is a complete 10-bit to 16-bit resolution tracking
> + resolver-to-digital converter, integrating an on-board programmable
> + sinusoidal oscillator that provides sine wave excitation for
> + resolvers.
> +
> + The AD2S1210 allows the user to read the angular position or the
> + angular velocity data directly from the parallel outputs or through
> + the serial interface.
> +
> + The mode of operation of the communication channel (parallel or serial) is
> + selected by the A0 and A1 input pins. In normal mode, data is latched by
> + toggling the SAMPLE line and can then be read directly. In configuration mode,
> + data is read or written using a register access scheme (address byte with
> + read/write flag and data byte).
> +
> + A1 A0 Result
> + 0 0 Normal mode - position output
> + 0 1 Normal mode - velocity output
> + 1 0 Reserved
> + 1 1 Configuration mode
> +
> + In normal mode, the resolution of the digital output is selected using
> + the RES0 and RES1 input pins. In configuration mode, the resolution is
> + selected by setting the RES0 and RES1 bits in the control register.
> +
> + RES1 RES0 Resolution (Bits)
> + 0 0 10
> + 0 1 12
> + 1 0 14
> + 1 1 16
> +
> + Note on SPI connections: The CS line on the AD2S1210 should hard-wired to
> + logic low and the WR/FSYNC line on the AD2S1210 should be connected to the
> + SPI CSn output of the SPI controller.
> +
> + Datasheet:
> + https://www.analog.com/media/en/technical-documentation/data-sheets/ad2s1210.pdf
> +
> +properties:
> + compatible:
> + const: adi,ad2s1210
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 25000000
> +
> + spi-cpha: true
> +
> + avdd-supply:
> + description:
> + A 4.75 to 5.25 V regulator that powers the Analog Supply Voltage (AVDD)
> + pin.
> +
> + dvdd-supply:
> + description:
> + A 4.75 to 5.25 V regulator that powers the Digital Supply Voltage (DVDD)
> + pin.
> +
> + vdrive-supply:
> + description:
> + A 2.3 to 5.25 V regulator that powers the Logic Power Supply Input
> + (VDrive) pin.
> +
> + clocks:
> + maxItems: 1
> + description: External oscillator clock (CLKIN).
> +
> + reset-gpios:
> + description:
> + GPIO connected to the /RESET pin. As the line needs to be low for the
> + reset to be active, it should be configured as GPIO_ACTIVE_LOW.
> + maxItems: 1
> +
> + sample-gpios:
> + description:
> + GPIO connected to the /SAMPLE pin. As the line needs to be low to trigger
> + a sample, it should be configured as GPIO_ACTIVE_LOW.
> + maxItems: 1
> +
> + mode-gpios:
> + description:
> + GPIO lines connected to the A0 and A1 pins. These pins select the data
> + transfer mode.
> + minItems: 2
> + maxItems: 2
> +
> + resolution-gpios:
> + description:
> + GPIO lines connected to the RES0 and RES1 pins. These pins select the
> + resolution of the digital output. If omitted, it is assumed that the
> + RES0 and RES1 pins are hard-wired to match the assigned-resolution-bits
> + property.
> + minItems: 2
> + maxItems: 2
> +
> + fault-gpios:
> + description:
> + GPIO lines connected to the LOT and DOS pins. These pins combined indicate
> + the type of fault present, if any. As these pins a pulled low to indicate
> + a fault condition, they should be configured as GPIO_ACTIVE_LOW.
> + minItems: 2
> + maxItems: 2
> +
> + adi,fixed-mode:
> + description:
> + This is used to indicate the selected mode if A0 and A1 are hard-wired
> + instead of connected to GPIOS (i.e. mode-gpios is omitted).
> + $ref: /schemas/types.yaml#/definitions/string
> + enum: [config, velocity, position]
> +
> + assigned-resolution-bits:
> + description:
> + Resolution of the digital output required by the application. This
> + determines the precision of the angle and/or the maximum speed that can
> + be measured. If resolution-gpios is omitted, it is assumed that RES0 and
> + RES1 are hard-wired to match this value.
> + enum: [10, 12, 14, 16]
> +
> +required:
> + - compatible
> + - reg
> + - spi-cpha
> + - avdd-supply
> + - dvdd-supply
> + - vdrive-supply
> + - clocks
> + - sample-gpios
> + - assigned-resolution-bits
> +
> +oneOf:
> + - required:
> + - mode-gpios
> + - required:
> + - adi,fixed-mode
> +
> +allOf:
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/gpio/gpio.h>
> +
> + spi {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + resolver@0 {
> + compatible = "adi,ad2s1210";
> + reg = <0>;
> + spi-max-frequency = <20000000>;
> + spi-cpha;
> + avdd-supply = <&avdd_regulator>;
> + dvdd-supply = <&dvdd_regulator>;
> + vdrive-supply = <&vdrive_regulator>;
> + clocks = <&ext_osc>;
> + sample-gpios = <&gpio0 90 GPIO_ACTIVE_LOW>;
> + mode-gpios = <&gpio0 86 0>, <&gpio0 87 0>;
> + resolution-gpios = <&gpio0 88 0>, <&gpio0 89 0>;
> + assigned-resolution-bits = <16>;
> + };
> + };
>
On Fri, 29 Sep 2023 12:23:11 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> There are quite a few imports and we will be adding more so it will
> make it easier to read if they are sorted.
>
> Signed-off-by: David Lechner <[email protected]>
Applied
> ---
>
> v3 changes:
> * This is a new patch split out from "staging: iio: resolver: ad2s1210:
> use devicetree to get fclkin"
>
> drivers/staging/iio/resolver/ad2s1210.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 28015322f562..832f86bf15e5 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -4,16 +4,16 @@
> *
> * Copyright (c) 2010-2010 Analog Devices Inc.
> */
> -#include <linux/types.h>
> -#include <linux/mutex.h>
> +#include <linux/delay.h>
> #include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/of.h>
> -#include <linux/spi/spi.h>
> #include <linux/slab.h>
> +#include <linux/spi/spi.h>
> #include <linux/sysfs.h>
> -#include <linux/delay.h>
> -#include <linux/gpio/consumer.h>
> -#include <linux/module.h>
> +#include <linux/types.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
>
On Fri, 29 Sep 2023 12:23:13 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> This adds an implementation of IIO_CHAN_INFO_SCALE to the ad2s1210
> resolver driver. This allows userspace to get the scale factor for the
> raw values.
>
> Signed-off-by: David Lechner <[email protected]>
Applied
Thanks,
> ---
>
> v3 changes:
> * Split ad2s1210_read_raw() into two functions to reduce complexity.
> * Use early return instead of break in switch statements.
>
> drivers/staging/iio/resolver/ad2s1210.c | 53 ++++++++++++++++++++++++++++-----
> 1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index f9774dff2df4..a710598a64f0 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -461,13 +461,10 @@ static ssize_t ad2s1210_store_reg(struct device *dev,
> return ret < 0 ? ret : len;
> }
>
> -static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> - struct iio_chan_spec const *chan,
> - int *val,
> - int *val2,
> - long m)
> +static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> + struct iio_chan_spec const *chan,
> + int *val)
> {
> - struct ad2s1210_state *st = iio_priv(indio_dev);
> int ret = 0;
>
> mutex_lock(&st->lock);
> @@ -514,6 +511,44 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> return ret;
> }
>
> +static const int ad2s1210_velocity_scale[] = {
> + 17089132, /* 8.192MHz / (2*pi * 2500 / 2^15) */
> + 42722830, /* 8.192MHz / (2*pi * 1000 / 2^15) */
> + 85445659, /* 8.192MHz / (2*pi * 500 / 2^15) */
> + 341782638, /* 8.192MHz / (2*pi * 125 / 2^15) */
> +};
> +
> +static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val,
> + int *val2,
> + long mask)
> +{
> + struct ad2s1210_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + return ad2s1210_single_conversion(st, chan, val);
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_ANGL:
> + /* approx 0.3 arc min converted to radians */
> + *val = 0;
> + *val2 = 95874;
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_ANGL_VEL:
> + *val = st->fclkin;
> + *val2 = ad2s1210_velocity_scale[st->resolution];
> + return IIO_VAL_FRACTIONAL;
> + default:
> + return -EINVAL;
> + }
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static IIO_DEVICE_ATTR(fclkin, 0644,
> ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0);
> static IIO_DEVICE_ATTR(fexcit, 0644,
> @@ -552,12 +587,14 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> .type = IIO_ANGL,
> .indexed = 1,
> .channel = 0,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> }, {
> .type = IIO_ANGL_VEL,
> .indexed = 1,
> .channel = 0,
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE),
> }
> };
>
>
On Fri, 29 Sep 2023 12:23:17 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> This removes the config register sysfs attribute.
>
> Writing to the config register directly can be dangerous and userspace
> should not need to have to know the register layout. This register
> can still be accessed though debugfs if needed.
>
> We can add new attributes to set specific flags in the config register
> in the future if needed (e.g. `enable_hysterisis` and
> `phase_lock_range`).
>
> Signed-off-by: David Lechner <[email protected]>
Applied.
On Fri, 29 Sep 2023 12:23:19 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> The AD2S1210 resolver has a hysteresis feature that can be used to
> prevent flicker in the LSB of the position register. This can be either
> enabled or disabled. Disabling hysteresis is useful for increasing
> precision by oversampling.
>
> Signed-off-by: David Lechner <[email protected]>
One trivial thing inline.
Thanks,
Jonathan
> ---
>
> v3 changes:
> * Refactored into more functions to reduce complexity of switch statements.
> * Use early return instead of break in switch statements.
>
> drivers/staging/iio/resolver/ad2s1210.c | 86 +++++++++++++++++++++++++++++++--
> 1 file changed, 83 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 0ec3598b600a..a82cb124a12f 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -78,7 +78,6 @@ struct ad2s1210_state {
> /** The external oscillator frequency in Hz. */
> unsigned long clkin_hz;
> unsigned int fexcit;
> - bool hysteresis;
> u8 resolution;
> /** For reading raw sample value via SPI. */
> __be16 sample __aligned(IIO_DMA_MINALIGN);
> @@ -430,6 +429,35 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> return ret;
> }
>
> +static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val)
> +{
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_test_bits(st->regmap, AD2S1210_REG_CONTROL,
> + AD2S1210_ENABLE_HYSTERESIS);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = !!ret;
regmap_test_bits() is documented as returning 1 or 0 anyway.
So this !! isn't necessary..
> + return IIO_VAL_INT;
> +}
> +
> +static int ad2s1210_set_hysteresis(struct ad2s1210_state *st, int val)
> +{
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
> + AD2S1210_ENABLE_HYSTERESIS,
> + val ? AD2S1210_ENABLE_HYSTERESIS : 0);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> static const int ad2s1210_velocity_scale[] = {
> 17089132, /* 8.192MHz / (2*pi * 2500 / 2^15) */
> 42722830, /* 8.192MHz / (2*pi * 1000 / 2^15) */
> @@ -462,7 +490,55 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_CHAN_INFO_HYSTERESIS:
> + switch (chan->type) {
> + case IIO_ANGL:
> + return ad2s1210_get_hysteresis(st, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int ad2s1210_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type,
> + int *length, long mask)
> +{
> + static const int hysteresis_available[] = { 0, 1 };
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HYSTERESIS:
> + switch (chan->type) {
> + case IIO_ANGL:
> + *vals = hysteresis_available;
> + *type = IIO_VAL_INT;
> + *length = ARRAY_SIZE(hysteresis_available);
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
>
> +static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct ad2s1210_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HYSTERESIS:
> + switch (chan->type) {
> + case IIO_ANGL:
> + return ad2s1210_set_hysteresis(st, val);
> + default:
> + return -EINVAL;
> + }
> default:
> return -EINVAL;
> }
> @@ -503,7 +579,10 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> .indexed = 1,
> .channel = 0,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> - BIT(IIO_CHAN_INFO_SCALE),
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_HYSTERESIS),
> + .info_mask_separate_available =
> + BIT(IIO_CHAN_INFO_HYSTERESIS),
> }, {
> .type = IIO_ANGL_VEL,
> .indexed = 1,
> @@ -581,6 +660,8 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
>
> static const struct iio_info ad2s1210_info = {
> .read_raw = ad2s1210_read_raw,
> + .read_avail = ad2s1210_read_avail,
> + .write_raw = ad2s1210_write_raw,
> .attrs = &ad2s1210_attribute_group,
> .debugfs_reg_access = &ad2s1210_debugfs_reg_access,
> };
> @@ -696,7 +777,6 @@ static int ad2s1210_probe(struct spi_device *spi)
>
> mutex_init(&st->lock);
> st->sdev = spi;
> - st->hysteresis = true;
> st->resolution = 12;
> st->fexcit = AD2S1210_DEF_EXCIT;
>
>
On Fri, 29 Sep 2023 12:23:30 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> The AD2S1210 has a programmable threshold for the degradation of signal
> (DOS) mismatch fault. This fault is triggered when the difference in
> amplitude between the sine and cosine inputs exceeds the threshold.
>
> The DOS reset min/max registers on the chip provide initial values
> for internal tracking of the min/max of the monitor signal after the
> fault register is cleared.
>
> This patch converts the custom device DOS reset min/max threshold
> attributes custom event attributes on the monitor signal channel.
>
> The attributes now use millivolts instead of the raw register value in
> accordance with the IIO ABI.
>
> Emitting the event will be implemented in a later patch.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v3 changes: This is a new patch in v3
>
> .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 ++++++
> drivers/staging/iio/resolver/ad2s1210.c | 99 ++++++++++++----------
> 2 files changed, 82 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
> new file mode 100644
> index 000000000000..ea75881b0c77
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
> @@ -0,0 +1,27 @@
> +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max
Ah. So these are differential. But the mismatch channel value isn't?
I also got the format wrong for differential channels. Oops. Should
be the in_altvoltage0-altvoltage1 format for the previous suggestion
to change that channel type to differential.
This looks fine to me as new ABI.
Jonathan
> +KernelVersion: 6.7
> +Contact: [email protected]
> +Description:
> + Reading returns the current Degradation of Signal Reset Maximum
> + Threshold value in millivolts. Writing sets the value.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max_available
> +KernelVersion: 6.7
> +Contact: [email protected]
> +Description:
> + Reading returns the allowable voltage range for
> + in_altvoltage0-altvoltage1_thresh_rising_reset_max.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min
> +KernelVersion: 6.7
> +Contact: [email protected]
> +Description:
> + Reading returns the current Degradation of Signal Reset Minimum
> + Threshold value in millivolts. Writing sets the value.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min_available
> +KernelVersion: 6.7
> +Contact: [email protected]
> +Description:
> + Reading returns the allowable voltage range for
> + in_altvoltage0-altvoltage1_thresh_rising_reset_min.
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index aa14edbe8a77..e1c95ec73545 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -283,41 +283,6 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
> return ret < 0 ? ret : len;
> }
>
> -static ssize_t ad2s1210_show_reg(struct device *dev,
> - struct device_attribute *attr,
> - char *buf)
> -{
> - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> - struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> - unsigned int value;
> - int ret;
> -
> - mutex_lock(&st->lock);
> - ret = regmap_read(st->regmap, iattr->address, &value);
> - mutex_unlock(&st->lock);
> -
> - return ret < 0 ? ret : sprintf(buf, "%d\n", value);
> -}
> -
> -static ssize_t ad2s1210_store_reg(struct device *dev,
> - struct device_attribute *attr,
> - const char *buf, size_t len)
> -{
> - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> - unsigned char data;
> - int ret;
> - struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> -
> - ret = kstrtou8(buf, 10, &data);
> - if (ret)
> - return -EINVAL;
> -
> - mutex_lock(&st->lock);
> - ret = regmap_write(st->regmap, iattr->address, data);
> - mutex_unlock(&st->lock);
> - return ret < 0 ? ret : len;
> -}
> -
> static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> struct iio_chan_spec const *chan,
> int *val)
> @@ -743,13 +708,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> static IIO_DEVICE_ATTR(fault, 0644,
> ad2s1210_show_fault, ad2s1210_clear_fault, 0);
>
> -static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
> - ad2s1210_show_reg, ad2s1210_store_reg,
> - AD2S1210_REG_DOS_RST_MAX_THRD);
> -static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
> - ad2s1210_show_reg, ad2s1210_store_reg,
> - AD2S1210_REG_DOS_RST_MIN_THRD);
> -
> static const struct iio_event_spec ad2s1210_position_event_spec[] = {
> {
> /* Tracking error exceeds LOT threshold fault. */
> @@ -867,8 +825,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
>
> static struct attribute *ad2s1210_attributes[] = {
> &iio_dev_attr_fault.dev_attr.attr,
> - &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
> - &iio_dev_attr_dos_rst_min_thrd.dev_attr.attr,
> NULL,
> };
>
> @@ -876,6 +832,49 @@ static const struct attribute_group ad2s1210_attribute_group = {
> .attrs = ad2s1210_attributes,
> };
>
> +static ssize_t event_attr_voltage_reg_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> + struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> + unsigned int value;
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_read(st->regmap, iattr->address, &value);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%d\n", value * THRESHOLD_MILLIVOLT_PER_LSB);
> +}
> +
> +static ssize_t event_attr_voltage_reg_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> + struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> + u16 data;
> + int ret;
> +
> + ret = kstrtou16(buf, 10, &data);
> + if (ret)
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_write(st->regmap, iattr->address,
> + data / THRESHOLD_MILLIVOLT_PER_LSB);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> static ssize_t
> in_angl1_thresh_rising_value_available_show(struct device *dev,
> struct device_attribute *attr,
> @@ -906,6 +905,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
> IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
> IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR);
> IIO_CONST_ATTR(in_altvoltage0_mag_value_available, THRESHOLD_RANGE_STR);
> +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_max, 0644,
> + event_attr_voltage_reg_show, event_attr_voltage_reg_store,
> + AD2S1210_REG_DOS_RST_MAX_THRD);
> +IIO_CONST_ATTR(in_altvoltage0_mag_reset_max_available, THRESHOLD_RANGE_STR);
> +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_min, 0644,
> + event_attr_voltage_reg_show, event_attr_voltage_reg_store,
> + AD2S1210_REG_DOS_RST_MIN_THRD);
> +IIO_CONST_ATTR(in_altvoltage0_mag_reset_min_available, THRESHOLD_RANGE_STR);
> IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
>
> @@ -914,6 +921,10 @@ static struct attribute *ad2s1210_event_attributes[] = {
> &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
> &iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr,
> &iio_const_attr_in_altvoltage0_mag_value_available.dev_attr.attr,
> + &iio_dev_attr_in_altvoltage0_mag_reset_max.dev_attr.attr,
> + &iio_const_attr_in_altvoltage0_mag_reset_max_available.dev_attr.attr,
> + &iio_dev_attr_in_altvoltage0_mag_reset_min.dev_attr.attr,
> + &iio_const_attr_in_altvoltage0_mag_reset_min_available.dev_attr.attr,
> &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
> &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
> NULL,
>
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This combines the ad2s1210_update_frequency_control_word() and
ad2s1210_soft_reset() functions into a single function since they
both have to be called together. (The software reset does not reset
any configuration registers, it only updates the excitation output
and resets the tracking loop.)
Also clean up a few things while touching this:
- move AD2S1210_DEF_EXCIT macro with similar macros
- remove unnecessary dev_err() calls
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* Expanded comment on soft reset register write.
* Fixed multiline comment style.
drivers/staging/iio/resolver/ad2s1210.c | 66 +++++++++++++++++----------------
1 file changed, 34 insertions(+), 32 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index a82cb124a12f..28ab877e1bc0 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -51,12 +51,11 @@
#define AD2S1210_MIN_CLKIN 6144000
#define AD2S1210_MAX_CLKIN 10240000
#define AD2S1210_MIN_EXCIT 2000
+#define AD2S1210_DEF_EXCIT 10000
#define AD2S1210_MAX_EXCIT 20000
#define AD2S1210_MIN_FCW 0x4
#define AD2S1210_MAX_FCW 0x50
-#define AD2S1210_DEF_EXCIT 10000
-
enum ad2s1210_mode {
MOD_POS = 0b00,
MOD_VEL = 0b01,
@@ -188,18 +187,32 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
return 0;
}
-static inline
-int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st)
+/*
+ * Sets the excitation frequency and performs software reset.
+ *
+ * Must be called with lock held.
+ */
+static int ad2s1210_reinit_excitation_frequency(struct ad2s1210_state *st,
+ u16 fexcit)
{
- unsigned char fcw;
+ int ret;
+ u8 fcw;
- fcw = (unsigned char)(st->fexcit * (1 << 15) / st->clkin_hz);
- if (fcw < AD2S1210_MIN_FCW || fcw > AD2S1210_MAX_FCW) {
- dev_err(&st->sdev->dev, "ad2s1210: FCW out of range\n");
+ fcw = fexcit * (1 << 15) / st->clkin_hz;
+ if (fcw < AD2S1210_MIN_FCW || fcw > AD2S1210_MAX_FCW)
return -ERANGE;
- }
- return regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw);
+ ret = regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw);
+ if (ret < 0)
+ return ret;
+
+ st->fexcit = fexcit;
+
+ /*
+ * Software reset reinitializes the excitation frequency output.
+ * It does not reset any of the configuration registers.
+ */
+ return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0);
}
static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
@@ -214,11 +227,6 @@ static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
bitmap);
}
-static inline int ad2s1210_soft_reset(struct ad2s1210_state *st)
-{
- return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0);
-}
-
static ssize_t ad2s1210_show_fexcit(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -233,27 +241,24 @@ static ssize_t ad2s1210_store_fexcit(struct device *dev,
const char *buf, size_t len)
{
struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- unsigned int fexcit;
+ u16 fexcit;
int ret;
- ret = kstrtouint(buf, 10, &fexcit);
- if (ret < 0)
- return ret;
- if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) {
- dev_err(dev,
- "ad2s1210: excitation frequency out of range\n");
+ ret = kstrtou16(buf, 10, &fexcit);
+ if (ret < 0 || fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT)
return -EINVAL;
- }
+
mutex_lock(&st->lock);
- st->fexcit = fexcit;
- ret = ad2s1210_update_frequency_control_word(st);
+ ret = ad2s1210_reinit_excitation_frequency(st, fexcit);
if (ret < 0)
goto error_ret;
- ret = ad2s1210_soft_reset(st);
+
+ ret = len;
+
error_ret:
mutex_unlock(&st->lock);
- return ret < 0 ? ret : len;
+ return ret;
}
static ssize_t ad2s1210_show_resolution(struct device *dev,
@@ -630,10 +635,8 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
if (ret < 0)
goto error_ret;
- ret = ad2s1210_update_frequency_control_word(st);
- if (ret < 0)
- goto error_ret;
- ret = ad2s1210_soft_reset(st);
+ ret = ad2s1210_reinit_excitation_frequency(st, AD2S1210_DEF_EXCIT);
+
error_ret:
mutex_unlock(&st->lock);
return ret;
@@ -778,7 +781,6 @@ static int ad2s1210_probe(struct spi_device *spi)
mutex_init(&st->lock);
st->sdev = spi;
st->resolution = 12;
- st->fexcit = AD2S1210_DEF_EXCIT;
ret = ad2s1210_setup_clocks(st);
if (ret < 0)
--
2.42.0
On Fri, 29 Sep 2023 12:23:31 -0500
David Lechner <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> When reading the position and velocity on the AD2S1210, there is also a
> 3rd byte following the two data bytes that contains the fault flag bits.
> This patch adds support for reading this byte and generating events when
> faults occur.
>
> The faults are mapped to various channels and event types in order to
> have a unique event for each fault.
>
> Signed-off-by: David Lechner <[email protected]>
Use of x and y modifiers is a little odd. What was your reasoning?
Was it just that there was a X_OR_Y modifier? If so, don't use that!
It seemed like a good idea at the time, but it's not nice to deal with
and requires a channel with that modifier to hang the controls off
+ make sure userspace expects that event code.
> ---
>
> v3 changes: This is a new patch in v3
>
> drivers/staging/iio/resolver/ad2s1210.c | 175 +++++++++++++++++++++++++++++---
> 1 file changed, 161 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index e1c95ec73545..dc3cc3ab855e 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -21,6 +21,7 @@
> #include <linux/types.h>
>
> #include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/trigger_consumer.h>
> @@ -35,6 +36,16 @@
> #define AD2S1210_SET_ENRES GENMASK(3, 2)
> #define AD2S1210_SET_RES GENMASK(1, 0)
>
> +/* fault register flags */
> +#define AD2S1210_FAULT_CLIP BIT(7)
> +#define AD2S1210_FAULT_LOS BIT(6)
> +#define AD2S1210_FAULT_DOS_OVR BIT(5)
> +#define AD2S1210_FAULT_DOS_MIS BIT(4)
> +#define AD2S1210_FAULT_LOT BIT(3)
> +#define AD2S1210_FAULT_VELOCITY BIT(2)
> +#define AD2S1210_FAULT_PHASE BIT(1)
> +#define AD2S1210_FAULT_CONFIG_PARITY BIT(0)
> +
> #define AD2S1210_REG_POSITION_MSB 0x80
> #define AD2S1210_REG_POSITION_LSB 0x81
> #define AD2S1210_REG_VELOCITY_MSB 0x82
> @@ -71,6 +82,8 @@
> /* max voltage for threshold registers is 0x7F * 38 mV */
> #define THRESHOLD_RANGE_STR "[0 38 4826]"
>
> +#define FAULT_ONESHOT(bit, new, old) (new & bit && !(old & bit))
> +
> enum ad2s1210_mode {
> MOD_POS = 0b00,
> MOD_VEL = 0b01,
> @@ -98,8 +111,13 @@ struct ad2s1210_state {
> unsigned long clkin_hz;
> /** The selected resolution */
> enum ad2s1210_resolution resolution;
> + /** Copy of fault register from the previous read. */
> + u8 prev_fault_flags;
> /** For reading raw sample value via SPI. */
> - __be16 sample __aligned(IIO_DMA_MINALIGN);
> + struct {
> + __be16 raw;
> + u8 fault;
> + } sample __aligned(IIO_DMA_MINALIGN);;
> /** Scan buffer */
> struct {
> __be16 chan[2];
> @@ -158,7 +176,15 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg,
> if (ret < 0)
> return ret;
>
> - return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
> + ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
> + if (ret < 0)
> + return ret;
> +
> + /* soft reset also clears the fault register */
> + if (reg == AD2S1210_REG_SOFT_RESET)
> + st->prev_fault_flags = 0;
> +
> + return 0;
> }
>
> /*
> @@ -200,6 +226,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
> if (ret < 0)
> return ret;
>
> + /* reading the fault register also clears it */
> + if (reg == AD2S1210_REG_FAULT)
> + st->prev_fault_flags = 0;
> +
> /*
> * If the D7 bit is set on any read/write register, it indicates a
> * parity error. The fault register is read-only and the D7 bit means
> @@ -283,14 +313,92 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
> return ret < 0 ? ret : len;
> }
>
> -static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> +static void ad2s1210_push_events(struct iio_dev *indio_dev,
> + u8 flags, s64 timestamp)
> +{
> + struct ad2s1210_state *st = iio_priv(indio_dev);
> +
> + /* Sine/cosine inputs clipped */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_CLIP, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_MOD_EVENT_CODE(IIO_ALTVOLTAGE, 1,
> + IIO_MOD_X_OR_Y,
Hmm. So this is a weird corner I'd forgotten about and explains your
use of X and Y modifiers.
Long ago this was added to support a similar case to the one you have,
but mixing and matching modifiers like this doesn't scale, so
I think we'd be better just signally events on both channels.
If nothing else, someone waiting for an event on a specific channel
isn't looking for this weird modifier.
When it was used before we added a channel for MOD_X_OR_Y so events
were enabled on that. It's horrible though so don't do that.
> + IIO_EV_TYPE_MAG,
> + IIO_EV_DIR_NONE),
> + timestamp);
> +
> + /* Sine/cosine inputs below LOS threshold */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_LOS, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + timestamp);
> +
> + /* Sine/cosine inputs exceed DOS overrange threshold */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_OVR, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> +
> + /* Sine/cosine inputs exceed DOS mismatch threshold */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_MIS, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0,
> + IIO_EV_TYPE_MAG,
> + IIO_EV_DIR_NONE),
> + timestamp);
> +
> + /* Tracking error exceeds LOT threshold */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_LOT, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_ANGL, 1,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> +
> + /* Velocity exceeds maximum tracking rate */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_VELOCITY, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_ANGL_VEL, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + timestamp);
> +
> + /* Phase error exceeds phase lock range */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_PHASE, flags, st->prev_fault_flags))
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_PHASE, 0,
> + IIO_EV_TYPE_MAG,
> + IIO_EV_DIR_NONE),
> + timestamp);
> +
> + /* Configuration parity error */
> + if (FAULT_ONESHOT(AD2S1210_FAULT_CONFIG_PARITY, flags,
> + st->prev_fault_flags))
> + /*
> + * Userspace should also get notified of this via error return
> + * when trying to write to any attribute that writes a register.
> + */
> + dev_err_ratelimited(&indio_dev->dev,
> + "Configuration parity error\n");
> +
> + st->prev_fault_flags = flags;
> +}
> +
> +static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan,
> int *val)
> {
> + struct ad2s1210_state *st = iio_priv(indio_dev);
> + s64 timestamp;
> int ret;
>
> mutex_lock(&st->lock);
> gpiod_set_value(st->sample_gpio, 1);
> + timestamp = iio_get_time_ns(indio_dev);
> /* delay (6 * tck + 20) nano seconds */
> udelay(1);
>
> @@ -307,17 +415,17 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> }
> if (ret < 0)
> goto error_ret;
> - ret = spi_read(st->sdev, &st->sample, 2);
> + ret = spi_read(st->sdev, &st->sample, 3);
> if (ret < 0)
> goto error_ret;
>
> switch (chan->type) {
> case IIO_ANGL:
> - *val = be16_to_cpu(st->sample);
> + *val = be16_to_cpu(st->sample.raw);
> ret = IIO_VAL_INT;
> break;
> case IIO_ANGL_VEL:
> - *val = (s16)be16_to_cpu(st->sample);
> + *val = (s16)be16_to_cpu(st->sample.raw);
> ret = IIO_VAL_INT;
> break;
> default:
> @@ -325,6 +433,8 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> break;
> }
>
> + ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
> +
> error_ret:
> gpiod_set_value(st->sample_gpio, 0);
> /* delay (2 * tck + 20) nano seconds */
> @@ -608,7 +718,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - return ad2s1210_single_conversion(st, chan, val);
> + return ad2s1210_single_conversion(indio_dev, chan, val);
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_ANGL:
> @@ -721,6 +831,14 @@ static const struct iio_event_spec ad2s1210_position_event_spec[] = {
> },
> };
>
> +static const struct iio_event_spec ad2s1210_velocity_event_spec[] = {
> + {
> + /* Velocity exceeds maximum tracking rate fault. */
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + },
> +};
> +
> static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
> {
> /* Phase error fault. */
> @@ -754,6 +872,14 @@ static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
> },
> };
>
> +static const struct iio_event_spec ad2s1210_sin_cos_event_spec[] = {
> + {
> + /* Sine/cosine clipping fault. */
> + .type = IIO_EV_TYPE_MAG,
> + .dir = IIO_EV_DIR_NONE,
> + },
> +};
> +
> static const struct iio_chan_spec ad2s1210_channels[] = {
> {
> .type = IIO_ANGL,
> @@ -784,6 +910,8 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> },
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> + .event_spec = ad2s1210_velocity_event_spec,
> + .num_event_specs = ARRAY_SIZE(ad2s1210_velocity_event_spec),
> },
> IIO_CHAN_SOFT_TIMESTAMP(2),
> {
> @@ -820,6 +948,26 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> .scan_index = -1,
> .event_spec = ad2s1210_monitor_signal_event_spec,
> .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
> + }, {
> + /* sine input */
> + .type = IIO_ALTVOLTAGE,
> + .indexed = 1,
> + .channel = 1,
> + .modified = 1,
> + .channel2 = IIO_MOD_Y,
A bit odd and I'm not sure it's beneficial over just using an index
and a label for the channel.
If a modifier is necessary then we could add a new one for this.
Another option might be to provide in_altvoltage0_phase
or... can we map this to i (inphase) and q (quadrature) phases?
Sort of feels like we can, and those modifiers already exist.
Note though that if we do that, we'll need to modify the various
ABI docs etc to include the modifiers whenever it's about the sine
and cosine channels.
> + .scan_index = -1,
> + .event_spec = ad2s1210_sin_cos_event_spec,
> + .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec),
> + }, {
> + /* cosine input */
> + .type = IIO_ALTVOLTAGE,
> + .indexed = 1,
> + .channel = 1,
> + .modified = 1,
> + .channel2 = IIO_MOD_X,
> + .scan_index = -1,
> + .event_spec = ad2s1210_sin_cos_event_spec,
> + .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec),
> },
> };
>
> @@ -936,7 +1084,7 @@ static const struct attribute_group ad2s1210_event_attribute_group = {
>
> static int ad2s1210_initial(struct ad2s1210_state *st)
> {
> - unsigned char data;
> + unsigned int data;
> int ret;
>
> mutex_lock(&st->lock);
> @@ -1073,12 +1221,11 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
> if (ret < 0)
> goto error_ret;
>
> - /* REVIST: we can read 3 bytes here and also get fault flags */
> - ret = spi_read(st->sdev, st->rx, 2);
> + ret = spi_read(st->sdev, &st->sample, 3);
> if (ret < 0)
> goto error_ret;
>
> - memcpy(&st->scan.chan[chan++], st->rx, 2);
> + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> }
>
> if (test_bit(1, indio_dev->active_scan_mask)) {
> @@ -1086,14 +1233,14 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
> if (ret < 0)
> goto error_ret;
>
> - /* REVIST: we can read 3 bytes here and also get fault flags */
> - ret = spi_read(st->sdev, st->rx, 2);
> + ret = spi_read(st->sdev, &st->sample, 3);
> if (ret < 0)
> goto error_ret;
>
> - memcpy(&st->scan.chan[chan++], st->rx, 2);
> + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
> }
>
> + ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
> iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp);
>
> error_ret:
>
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
The AD2S1210 chip has a phase lock range feature that allows selecting
the allowable phase difference between the excitation output and the
sine and cosine inputs. This can be set to either 44 degrees (default)
or 360 degrees.
This patch adds a new phase channel with a threshold event that can be
used to configure the phase lock range. Actually emitting the event
will be added in a subsequent patch.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* This is a new patch to replace "staging: iio: resolver: ad2s1210: add
phase_lock_range attributes"
drivers/staging/iio/resolver/ad2s1210.c | 125 ++++++++++++++++++++++++++++++++
1 file changed, 125 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index a0a426d0af19..bafc134eed97 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -56,6 +56,13 @@
#define AD2S1210_MIN_FCW 0x4
#define AD2S1210_MAX_FCW 0x50
+/* 44 degrees ~= 0.767945 radians */
+#define PHASE_44_DEG_TO_RAD_INT 0
+#define PHASE_44_DEG_TO_RAD_MICRO 767945
+/* 360 degrees ~= 6.283185 radians */
+#define PHASE_360_DEG_TO_RAD_INT 6
+#define PHASE_360_DEG_TO_RAD_MICRO 283185
+
enum ad2s1210_mode {
MOD_POS = 0b00,
MOD_VEL = 0b01,
@@ -376,6 +383,54 @@ static int ad2s1210_set_hysteresis(struct ad2s1210_state *st, int val)
return ret;
}
+static int ad2s1210_get_phase_lock_range(struct ad2s1210_state *st,
+ int *val, int *val2)
+{
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_test_bits(st->regmap, AD2S1210_REG_CONTROL,
+ AD2S1210_PHASE_LOCK_RANGE_44);
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ if (ret) {
+ /* 44 degrees as radians */
+ *val = PHASE_44_DEG_TO_RAD_INT;
+ *val2 = PHASE_44_DEG_TO_RAD_MICRO;
+ } else {
+ /* 360 degrees as radians */
+ *val = PHASE_360_DEG_TO_RAD_INT;
+ *val2 = PHASE_360_DEG_TO_RAD_MICRO;
+ }
+
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad2s1210_set_phase_lock_range(struct ad2s1210_state *st,
+ int val, int val2)
+{
+ int deg, ret;
+
+ /* convert radians to degrees - only two allowable values */
+ if (val == PHASE_44_DEG_TO_RAD_INT && val2 == PHASE_44_DEG_TO_RAD_MICRO)
+ deg = 44;
+ else if (val == PHASE_360_DEG_TO_RAD_INT &&
+ val2 == PHASE_360_DEG_TO_RAD_MICRO)
+ deg = 360;
+ else
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+ ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
+ AD2S1210_PHASE_LOCK_RANGE_44,
+ deg == 44 ? AD2S1210_PHASE_LOCK_RANGE_44 : 0);
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
{
unsigned int reg_val;
@@ -547,6 +602,16 @@ static IIO_DEVICE_ATTR(lot_low_thrd, 0644,
ad2s1210_show_reg, ad2s1210_store_reg,
AD2S1210_REG_LOT_LOW_THRD);
+static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
+ {
+ /* Phase error fault. */
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_NONE,
+ /* Phase lock range. */
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+};
+
static const struct iio_chan_spec ad2s1210_channels[] = {
{
.type = IIO_ANGL,
@@ -563,6 +628,14 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.channel = 0,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
+ }, {
+ /* used to configure phase lock range and get phase lock error */
+ .type = IIO_PHASE,
+ .indexed = 1,
+ .channel = 0,
+ .scan_index = -1,
+ .event_spec = ad2s1210_phase_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_phase_event_spec),
}, {
/* excitation frequency output */
.type = IIO_ALTVOLTAGE,
@@ -591,6 +664,21 @@ static const struct attribute_group ad2s1210_attribute_group = {
.attrs = ad2s1210_attributes,
};
+IIO_CONST_ATTR(in_phase0_mag_value_available,
+ __stringify(PHASE_44_DEG_TO_RAD_INT) "."
+ __stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
+ __stringify(PHASE_360_DEG_TO_RAD_INT) "."
+ __stringify(PHASE_360_DEG_TO_RAD_MICRO));
+
+static struct attribute *ad2s1210_event_attributes[] = {
+ &iio_const_attr_in_phase0_mag_value_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group ad2s1210_event_attribute_group = {
+ .attrs = ad2s1210_event_attributes,
+};
+
static int ad2s1210_initial(struct ad2s1210_state *st)
{
unsigned char data;
@@ -615,6 +703,40 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
return ret;
}
+static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int *val, int *val2)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+
+ switch (chan->type) {
+ case IIO_PHASE:
+ return ad2s1210_get_phase_lock_range(st, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ enum iio_event_info info,
+ int val, int val2)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+
+ switch (chan->type) {
+ case IIO_PHASE:
+ return ad2s1210_set_phase_lock_range(st, val, val2);
+ default:
+ return -EINVAL;
+ }
+}
+
static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int reg, unsigned int writeval,
unsigned int *readval)
@@ -635,10 +757,13 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
}
static const struct iio_info ad2s1210_info = {
+ .event_attrs = &ad2s1210_event_attribute_group,
.read_raw = ad2s1210_read_raw,
.read_avail = ad2s1210_read_avail,
.write_raw = ad2s1210_write_raw,
.attrs = &ad2s1210_attribute_group,
+ .read_event_value = ad2s1210_read_event_value,
+ .write_event_value = ad2s1210_write_event_value,
.debugfs_reg_access = &ad2s1210_debugfs_reg_access,
};
--
2.42.0
On Mon, 2 Oct 2023 11:02:40 +0300
Dan Carpenter <[email protected]> wrote:
> On Fri, Sep 29, 2023 at 12:23:06PM -0500, David Lechner wrote:
> > From: David Lechner <[email protected]>
> >
> > From: David Lechner <[email protected]>
> >
>
> Having two from headers kind of messes things up. The second From is
> included in the body of the commit message.
>
> regards,
> dan carpenter
Strangely b4 + git am didn't do that. I was assuming I'd need
to fix these by hand but didn't need to. I thanked my lucky
stars for a few mins saved and didn't look into why ;)
e.g.
https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/commit/?h=testing&id=b3689e14415a874630f0894d8c3ac7ea01603d57
BTW, David replied to the cover letter to call out this mess up.
On Fri, Sep 29, 2023 at 12:23:06PM -0500, David Lechner wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
Having two from headers kind of messes things up. The second From is
included in the body of the commit message.
regards,
dan carpenter
On Fri, 29 Sep 2023 12:23:06 -0500, David Lechner wrote:
> From: David Lechner <[email protected]>
>
> From: David Lechner <[email protected]>
>
> This adds new DeviceTree bindings for the Analog Devices, Inc. AD2S1210
> resolver-to-digital converter.
>
> Co-developed-by: Apelete Seketeli <[email protected]>
> Signed-off-by: Apelete Seketeli <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v3 changes:
> * Expanded top-level description of A0/A1 lines.
> * Added required voltage -supply properties. (I did not pick up Rob's
> Reviewed-by since I wasn't sure if this was trivial enough.)
>
> v2 changes:
> * Add Co-developed-by:
> * Remove extraneous quotes on strings
> * Remove extraneous pipe on some multi-line descriptions
>
> .../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++++++++++++++++++++
> 1 file changed, 177 insertions(+)
>
Reviewed-by: Rob Herring <[email protected]>
On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 29 Sep 2023 12:23:31 -0500
> David Lechner <[email protected]> wrote:
>
> > From: David Lechner <[email protected]>
> >
> > From: David Lechner <[email protected]>
> >
> > When reading the position and velocity on the AD2S1210, there is also a
> > 3rd byte following the two data bytes that contains the fault flag bits.
> > This patch adds support for reading this byte and generating events when
> > faults occur.
> >
> > The faults are mapped to various channels and event types in order to
> > have a unique event for each fault.
> >
> > Signed-off-by: David Lechner <[email protected]>
>
> Use of x and y modifiers is a little odd. What was your reasoning?
> Was it just that there was a X_OR_Y modifier? If so, don't use that!
> It seemed like a good idea at the time, but it's not nice to deal with
> and requires a channel with that modifier to hang the controls off
> + make sure userspace expects that event code.
>
Yes, I was perhaps getting a bit too creative here (the two inputs
come from transformers mounted 90 degrees apart so measure X and Y
component of an angle). The only reason I did this was to take
advantage of the possibility of an "OR" event to avoid double events
for a single fault bit. We can just go with the double event on the
two different input channels as discussed in "[PATCH v3 22/27]
staging: iio: resolver: ad2s1210: convert LOS threshold to event
attr".
On Mon, Oct 2, 2023 at 11:58 AM David Lechner <[email protected]> wrote:
>
> On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Fri, 29 Sep 2023 12:23:31 -0500
> > David Lechner <[email protected]> wrote:
> >
> > > From: David Lechner <[email protected]>
> > >
> > > From: David Lechner <[email protected]>
> > >
> > > When reading the position and velocity on the AD2S1210, there is also a
> > > 3rd byte following the two data bytes that contains the fault flag bits.
> > > This patch adds support for reading this byte and generating events when
> > > faults occur.
> > >
> > > The faults are mapped to various channels and event types in order to
> > > have a unique event for each fault.
> > >
> > > Signed-off-by: David Lechner <[email protected]>
> >
> > Use of x and y modifiers is a little odd. What was your reasoning?
> > Was it just that there was a X_OR_Y modifier? If so, don't use that!
> > It seemed like a good idea at the time, but it's not nice to deal with
> > and requires a channel with that modifier to hang the controls off
> > + make sure userspace expects that event code.
>
>
> Regarding the point about "requires a channel with that modifier to
> hang the controls off...". Although that comment was about modifiers,
> does it also apply in general.
>
> There are several fault events that don't have any configurable
> parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds
> max tracking rate_. So there won't be any attributes that contain the
> event specification for those (e.g. no `events/in_angl0_*`
> attributes). It sounds like this would be a problem as well?
>
> Should we consider a IIO_EV_INFO_LABEL so that we can have some sort
> of attribute (namely `events/<dir>_<channel spec>_label`) so that
> userspace can enumerate expected events for non-configurable events?
Well, I didn't think that all the way through before I hit send.
IIO_EV_INFO_LABEL is clearly not the right way to implement a
`events/*_label` attribute, but however we consider going about it,
the point of adding such an attribute was the main idea.
On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 29 Sep 2023 12:23:31 -0500
> David Lechner <[email protected]> wrote:
>
> > From: David Lechner <[email protected]>
> >
> > From: David Lechner <[email protected]>
> >
> > When reading the position and velocity on the AD2S1210, there is also a
> > 3rd byte following the two data bytes that contains the fault flag bits.
> > This patch adds support for reading this byte and generating events when
> > faults occur.
> >
> > The faults are mapped to various channels and event types in order to
> > have a unique event for each fault.
> >
> > Signed-off-by: David Lechner <[email protected]>
>
> Use of x and y modifiers is a little odd. What was your reasoning?
> Was it just that there was a X_OR_Y modifier? If so, don't use that!
> It seemed like a good idea at the time, but it's not nice to deal with
> and requires a channel with that modifier to hang the controls off
> + make sure userspace expects that event code.
Regarding the point about "requires a channel with that modifier to
hang the controls off...". Although that comment was about modifiers,
does it also apply in general.
There are several fault events that don't have any configurable
parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds
max tracking rate_. So there won't be any attributes that contain the
event specification for those (e.g. no `events/in_angl0_*`
attributes). It sounds like this would be a problem as well?
Should we consider a IIO_EV_INFO_LABEL so that we can have some sort
of attribute (namely `events/<dir>_<channel spec>_label`) so that
userspace can enumerate expected events for non-configurable events?
On Sat, Sep 30, 2023 at 10:47 AM Jonathan Cameron <[email protected]> wrote:
>
> On Fri, 29 Sep 2023 12:23:30 -0500
> David Lechner <[email protected]> wrote:
>
> > From: David Lechner <[email protected]>
> >
> > From: David Lechner <[email protected]>
> >
> > The AD2S1210 has a programmable threshold for the degradation of signal
> > (DOS) mismatch fault. This fault is triggered when the difference in
> > amplitude between the sine and cosine inputs exceeds the threshold.
> >
> > The DOS reset min/max registers on the chip provide initial values
> > for internal tracking of the min/max of the monitor signal after the
> > fault register is cleared.
> >
> > This patch converts the custom device DOS reset min/max threshold
> > attributes custom event attributes on the monitor signal channel.
> >
> > The attributes now use millivolts instead of the raw register value in
> > accordance with the IIO ABI.
> >
> > Emitting the event will be implemented in a later patch.
> >
> > Signed-off-by: David Lechner <[email protected]>
> > ---
> >
> > v3 changes: This is a new patch in v3
> >
> > .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 ++++++
> > drivers/staging/iio/resolver/ad2s1210.c | 99 ++++++++++++----------
> > 2 files changed, 82 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
> > new file mode 100644
> > index 000000000000..ea75881b0c77
> > --- /dev/null
> > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210
> > @@ -0,0 +1,27 @@
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max
> Ah. So these are differential. But the mismatch channel value isn't?
>
> I also got the format wrong for differential channels. Oops. Should
> be the in_altvoltage0-altvoltage1 format for the previous suggestion
> to change that channel type to differential.
>
> This looks fine to me as new ABI.
>
> Jonathan
>
As discussed in
<https://lore.kernel.org/linux-iio/CAMknhBFKSqXvgOeRjGAOfURzndmxmCffdU6MUirEmfzKqwM_Kg@mail.gmail.com/>,
technically no they are not differential. I started to implement it
that way but changed my mind after understanding the datasheet more in
depth. It looks like I forgot to update the documentation in this
patch to match the final implementation that was submitted.
In any case, I will update the documentation to match the
implementation or vice versa depending on what we decide on for which
channel the events should be associated with.
>
>
> > +KernelVersion: 6.7
> > +Contact: [email protected]
> > +Description:
> > + Reading returns the current Degradation of Signal Reset Maximum
> > + Threshold value in millivolts. Writing sets the value.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max_available
> > +KernelVersion: 6.7
> > +Contact: [email protected]
> > +Description:
> > + Reading returns the allowable voltage range for
> > + in_altvoltage0-altvoltage1_thresh_rising_reset_max.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min
> > +KernelVersion: 6.7
> > +Contact: [email protected]
> > +Description:
> > + Reading returns the current Degradation of Signal Reset Minimum
> > + Threshold value in millivolts. Writing sets the value.
> > +
> > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min_available
> > +KernelVersion: 6.7
> > +Contact: [email protected]
> > +Description:
> > + Reading returns the allowable voltage range for
> > + in_altvoltage0-altvoltage1_thresh_rising_reset_min.
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index aa14edbe8a77..e1c95ec73545 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -283,41 +283,6 @@ static ssize_t ad2s1210_clear_fault(struct device *dev,
> > return ret < 0 ? ret : len;
> > }
> >
> > -static ssize_t ad2s1210_show_reg(struct device *dev,
> > - struct device_attribute *attr,
> > - char *buf)
> > -{
> > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > - unsigned int value;
> > - int ret;
> > -
> > - mutex_lock(&st->lock);
> > - ret = regmap_read(st->regmap, iattr->address, &value);
> > - mutex_unlock(&st->lock);
> > -
> > - return ret < 0 ? ret : sprintf(buf, "%d\n", value);
> > -}
> > -
> > -static ssize_t ad2s1210_store_reg(struct device *dev,
> > - struct device_attribute *attr,
> > - const char *buf, size_t len)
> > -{
> > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > - unsigned char data;
> > - int ret;
> > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > -
> > - ret = kstrtou8(buf, 10, &data);
> > - if (ret)
> > - return -EINVAL;
> > -
> > - mutex_lock(&st->lock);
> > - ret = regmap_write(st->regmap, iattr->address, data);
> > - mutex_unlock(&st->lock);
> > - return ret < 0 ? ret : len;
> > -}
> > -
> > static int ad2s1210_single_conversion(struct ad2s1210_state *st,
> > struct iio_chan_spec const *chan,
> > int *val)
> > @@ -743,13 +708,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> > static IIO_DEVICE_ATTR(fault, 0644,
> > ad2s1210_show_fault, ad2s1210_clear_fault, 0);
> >
> > -static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
> > - ad2s1210_show_reg, ad2s1210_store_reg,
> > - AD2S1210_REG_DOS_RST_MAX_THRD);
> > -static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
> > - ad2s1210_show_reg, ad2s1210_store_reg,
> > - AD2S1210_REG_DOS_RST_MIN_THRD);
> > -
> > static const struct iio_event_spec ad2s1210_position_event_spec[] = {
> > {
> > /* Tracking error exceeds LOT threshold fault. */
> > @@ -867,8 +825,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> >
> > static struct attribute *ad2s1210_attributes[] = {
> > &iio_dev_attr_fault.dev_attr.attr,
> > - &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
> > - &iio_dev_attr_dos_rst_min_thrd.dev_attr.attr,
> > NULL,
> > };
> >
> > @@ -876,6 +832,49 @@ static const struct attribute_group ad2s1210_attribute_group = {
> > .attrs = ad2s1210_attributes,
> > };
> >
> > +static ssize_t event_attr_voltage_reg_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > + unsigned int value;
> > + int ret;
> > +
> > + mutex_lock(&st->lock);
> > + ret = regmap_read(st->regmap, iattr->address, &value);
> > + mutex_unlock(&st->lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return sprintf(buf, "%d\n", value * THRESHOLD_MILLIVOLT_PER_LSB);
> > +}
> > +
> > +static ssize_t event_attr_voltage_reg_store(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t len)
> > +{
> > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr);
> > + u16 data;
> > + int ret;
> > +
> > + ret = kstrtou16(buf, 10, &data);
> > + if (ret)
> > + return -EINVAL;
> > +
> > + mutex_lock(&st->lock);
> > + ret = regmap_write(st->regmap, iattr->address,
> > + data / THRESHOLD_MILLIVOLT_PER_LSB);
> > + mutex_unlock(&st->lock);
> > +
> > + if (ret < 0)
> > + return ret;
> > +
> > + return len;
> > +}
> > +
> > static ssize_t
> > in_angl1_thresh_rising_value_available_show(struct device *dev,
> > struct device_attribute *attr,
> > @@ -906,6 +905,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
> > IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
> > IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR);
> > IIO_CONST_ATTR(in_altvoltage0_mag_value_available, THRESHOLD_RANGE_STR);
> > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_max, 0644,
> > + event_attr_voltage_reg_show, event_attr_voltage_reg_store,
> > + AD2S1210_REG_DOS_RST_MAX_THRD);
> > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_max_available, THRESHOLD_RANGE_STR);
> > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_min, 0644,
> > + event_attr_voltage_reg_show, event_attr_voltage_reg_store,
> > + AD2S1210_REG_DOS_RST_MIN_THRD);
> > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_min_available, THRESHOLD_RANGE_STR);
> > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
These attribute names don't match the documentation above. :-/
> >
> > @@ -914,6 +921,10 @@ static struct attribute *ad2s1210_event_attributes[] = {
> > &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
> > &iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr,
> > &iio_const_attr_in_altvoltage0_mag_value_available.dev_attr.attr,
> > + &iio_dev_attr_in_altvoltage0_mag_reset_max.dev_attr.attr,
> > + &iio_const_attr_in_altvoltage0_mag_reset_max_available.dev_attr.attr,
> > + &iio_dev_attr_in_altvoltage0_mag_reset_min.dev_attr.attr,
> > + &iio_const_attr_in_altvoltage0_mag_reset_min_available.dev_attr.attr,
> > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
> > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
> > NULL,
> >
>
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-19-fa4364281745%40baylibre.com
patch subject: [PATCH v3 19/27] staging: iio: resolver: ad2s1210: add phase lock range support
config: i386-randconfig-062-20231003 (https://download.01.org/0day-ci/archive/20231003/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
>> drivers/staging/iio/resolver/ad2s1210.c:667:1: sparse: sparse: symbol 'iio_const_attr_in_phase0_mag_value_available' was not declared. Should it be static?
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi David,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-26-fa4364281745%40baylibre.com
patch subject: [PATCH v3 26/27] staging: iio: resolver: ad2s1210: implement fault events
config: x86_64-randconfig-161-20231002 (https://download.01.org/0day-ci/archive/20231003/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231003/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
| Closes: https://lore.kernel.org/r/[email protected]/
smatch warnings:
drivers/staging/iio/resolver/ad2s1210.c:436 ad2s1210_single_conversion() error: buffer overflow 'st->rx' 2 <= 2
vim +436 drivers/staging/iio/resolver/ad2s1210.c
ecf16f4922f691 David Lechner 2023-09-29 391 static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
29148543c52146 Jonathan Cameron 2011-10-05 392 struct iio_chan_spec const *chan,
e9336a85ceb885 David Lechner 2023-09-29 393 int *val)
817e5c65c511d4 Graf Yang 2010-10-27 394 {
ecf16f4922f691 David Lechner 2023-09-29 395 struct ad2s1210_state *st = iio_priv(indio_dev);
ecf16f4922f691 David Lechner 2023-09-29 396 s64 timestamp;
69cc7fbdcdf2e3 David Lechner 2023-09-29 397 int ret;
817e5c65c511d4 Graf Yang 2010-10-27 398
817e5c65c511d4 Graf Yang 2010-10-27 399 mutex_lock(&st->lock);
69cc7fbdcdf2e3 David Lechner 2023-09-29 400 gpiod_set_value(st->sample_gpio, 1);
ecf16f4922f691 David Lechner 2023-09-29 401 timestamp = iio_get_time_ns(indio_dev);
817e5c65c511d4 Graf Yang 2010-10-27 402 /* delay (6 * tck + 20) nano seconds */
817e5c65c511d4 Graf Yang 2010-10-27 403 udelay(1);
817e5c65c511d4 Graf Yang 2010-10-27 404
29148543c52146 Jonathan Cameron 2011-10-05 405 switch (chan->type) {
29148543c52146 Jonathan Cameron 2011-10-05 406 case IIO_ANGL:
69cc7fbdcdf2e3 David Lechner 2023-09-29 407 ret = ad2s1210_set_mode(st, MOD_POS);
29148543c52146 Jonathan Cameron 2011-10-05 408 break;
29148543c52146 Jonathan Cameron 2011-10-05 409 case IIO_ANGL_VEL:
69cc7fbdcdf2e3 David Lechner 2023-09-29 410 ret = ad2s1210_set_mode(st, MOD_VEL);
29148543c52146 Jonathan Cameron 2011-10-05 411 break;
29148543c52146 Jonathan Cameron 2011-10-05 412 default:
29148543c52146 Jonathan Cameron 2011-10-05 413 ret = -EINVAL;
29148543c52146 Jonathan Cameron 2011-10-05 414 break;
29148543c52146 Jonathan Cameron 2011-10-05 415 }
29148543c52146 Jonathan Cameron 2011-10-05 416 if (ret < 0)
29148543c52146 Jonathan Cameron 2011-10-05 417 goto error_ret;
ecf16f4922f691 David Lechner 2023-09-29 418 ret = spi_read(st->sdev, &st->sample, 3);
29148543c52146 Jonathan Cameron 2011-10-05 419 if (ret < 0)
817e5c65c511d4 Graf Yang 2010-10-27 420 goto error_ret;
29148543c52146 Jonathan Cameron 2011-10-05 421
29148543c52146 Jonathan Cameron 2011-10-05 422 switch (chan->type) {
29148543c52146 Jonathan Cameron 2011-10-05 423 case IIO_ANGL:
ecf16f4922f691 David Lechner 2023-09-29 424 *val = be16_to_cpu(st->sample.raw);
29148543c52146 Jonathan Cameron 2011-10-05 425 ret = IIO_VAL_INT;
29148543c52146 Jonathan Cameron 2011-10-05 426 break;
29148543c52146 Jonathan Cameron 2011-10-05 427 case IIO_ANGL_VEL:
ecf16f4922f691 David Lechner 2023-09-29 428 *val = (s16)be16_to_cpu(st->sample.raw);
29148543c52146 Jonathan Cameron 2011-10-05 429 ret = IIO_VAL_INT;
29148543c52146 Jonathan Cameron 2011-10-05 430 break;
29148543c52146 Jonathan Cameron 2011-10-05 431 default:
5e99f692d4e32e David Lechner 2023-09-21 432 ret = -EINVAL;
5e99f692d4e32e David Lechner 2023-09-21 433 break;
29148543c52146 Jonathan Cameron 2011-10-05 434 }
29148543c52146 Jonathan Cameron 2011-10-05 435
ecf16f4922f691 David Lechner 2023-09-29 @436 ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
^^^^^^
Apparently ->rx only has 2 elements.
ecf16f4922f691 David Lechner 2023-09-29 437
817e5c65c511d4 Graf Yang 2010-10-27 438 error_ret:
69cc7fbdcdf2e3 David Lechner 2023-09-29 439 gpiod_set_value(st->sample_gpio, 0);
817e5c65c511d4 Graf Yang 2010-10-27 440 /* delay (2 * tck + 20) nano seconds */
817e5c65c511d4 Graf Yang 2010-10-27 441 udelay(1);
817e5c65c511d4 Graf Yang 2010-10-27 442 mutex_unlock(&st->lock);
29148543c52146 Jonathan Cameron 2011-10-05 443 return ret;
817e5c65c511d4 Graf Yang 2010-10-27 444 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-25-fa4364281745%40baylibre.com
patch subject: [PATCH v3 25/27] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
config: i386-randconfig-062-20231003 (https://download.01.org/0day-ci/archive/20231004/[email protected]/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231004/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
sparse warnings: (new ones prefixed by >>)
drivers/staging/iio/resolver/ad2s1210.c:900:1: sparse: sparse: symbol 'iio_const_attr_in_phase0_mag_value_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:905:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_thresh_falling_value_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:906:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_thresh_rising_value_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:907:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_mag_value_available' was not declared. Should it be static?
>> drivers/staging/iio/resolver/ad2s1210.c:908:1: sparse: sparse: symbol 'iio_dev_attr_in_altvoltage0_mag_reset_max' was not declared. Should it be static?
>> drivers/staging/iio/resolver/ad2s1210.c:911:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_mag_reset_max_available' was not declared. Should it be static?
>> drivers/staging/iio/resolver/ad2s1210.c:912:1: sparse: sparse: symbol 'iio_dev_attr_in_altvoltage0_mag_reset_min' was not declared. Should it be static?
>> drivers/staging/iio/resolver/ad2s1210.c:915:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_mag_reset_min_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:916:1: sparse: sparse: symbol 'iio_dev_attr_in_angl1_thresh_rising_value_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:917:1: sparse: sparse: symbol 'iio_dev_attr_in_angl1_thresh_rising_hysteresis_available' was not declared. Should it be static?
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Samstag, 30. September 2023 16:34
> To: David Lechner <[email protected]>
> Cc: [email protected]; [email protected]; linux-
> [email protected]; David Lechner <[email protected]>; Rob Herring
> <[email protected]>; Krzysztof Kozlowski
> <[email protected]>; Conor Dooley <[email protected]>;
> Hennerich, Michael <[email protected]>; Sa, Nuno
> <[email protected]>; Axel Haslam <[email protected]>; Philip Molloy
> <[email protected]>; [email protected]; Apelete Seketeli
> <[email protected]>
> Subject: Re: [PATCH v3 01/27] dt-bindings: iio: resolver: add devicetree
> bindings for ad2s1210
>
>
> On Fri, 29 Sep 2023 12:23:06 -0500
> David Lechner <[email protected]> wrote:
>
> > From: David Lechner <[email protected]>
> >
> > From: David Lechner <[email protected]>
> >
> > This adds new DeviceTree bindings for the Analog Devices, Inc.
> > AD2S1210 resolver-to-digital converter.
> >
> > Co-developed-by: Apelete Seketeli <[email protected]>
> > Signed-off-by: Apelete Seketeli <[email protected]>
> > Signed-off-by: David Lechner <[email protected]>
>
> Michael, ideally I'd like your ack on this given it volunteers you as maintainer. If
> I don't hear I'm fine with leaving Michael listed because he's in MAINTAINERS
> anyway covering these bindings via a wild card entry:
Acked-by: Michael Hennerich <[email protected]>
>
> ANALOG DEVICES INC IIO DRIVERS
> M: Lars-Peter Clausen <[email protected]>
> M: Michael Hennerich <[email protected]>
> ...
> F: Documentation/devicetree/bindings/iio/*/adi,*
>
> So any queries should hit Michael anyway.
>
> LGTM but I'll also want the dt binding maintainers input before picking this up.
>
> Jonathan
>
> > ---
> >
> > v3 changes:
> > * Expanded top-level description of A0/A1 lines.
> > * Added required voltage -supply properties. (I did not pick up Rob's
> > Reviewed-by since I wasn't sure if this was trivial enough.)
> >
> > v2 changes:
> > * Add Co-developed-by:
> > * Remove extraneous quotes on strings
> > * Remove extraneous pipe on some multi-line descriptions
> >
> > .../bindings/iio/resolver/adi,ad2s1210.yaml | 177
> +++++++++++++++++++++
> > 1 file changed, 177 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
> > b/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
> > new file mode 100644
> > index 000000000000..8980b3cd8337
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/resolver/adi,ad2s1210.yaml
> > @@ -0,0 +1,177 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://urldefense.com/v3/__http://devicetree.org/schemas/iio/resolve
> >
> +r/adi,ad2s1210.yaml*__;Iw!!A3Ni8CS0y2Y!_mSGRdlDHlqAKev0r38paa3K51l2k
> G
> > +o8bShqK2TH4nAF_cYu2WixIa62xv0p-A70086DQmj4oN9FWvOlk78$
> > +$schema:
> > +https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.y
> >
> +aml*__;Iw!!A3Ni8CS0y2Y!_mSGRdlDHlqAKev0r38paa3K51l2kGo8bShqK2TH4n
> AF_c
> > +Yu2WixIa62xv0p-A70086DQmj4oN9FWzdE10U$
> > +
> > +title: Analog Devices AD2S1210 Resolver-to-Digital Converter
> > +
> > +maintainers:
> > + - Michael Hennerich <[email protected]>
> > +
> > +description: |
> > + The AD2S1210 is a complete 10-bit to 16-bit resolution tracking
> > + resolver-to-digital converter, integrating an on-board programmable
> > + sinusoidal oscillator that provides sine wave excitation for
> > + resolvers.
> > +
> > + The AD2S1210 allows the user to read the angular position or the
> > + angular velocity data directly from the parallel outputs or through
> > + the serial interface.
> > +
> > + The mode of operation of the communication channel (parallel or
> > + serial) is selected by the A0 and A1 input pins. In normal mode,
> > + data is latched by toggling the SAMPLE line and can then be read
> > + directly. In configuration mode, data is read or written using a
> > + register access scheme (address byte with read/write flag and data byte).
> > +
> > + A1 A0 Result
> > + 0 0 Normal mode - position output
> > + 0 1 Normal mode - velocity output
> > + 1 0 Reserved
> > + 1 1 Configuration mode
> > +
> > + In normal mode, the resolution of the digital output is selected
> > + using the RES0 and RES1 input pins. In configuration mode, the
> > + resolution is selected by setting the RES0 and RES1 bits in the control
> register.
> > +
> > + RES1 RES0 Resolution (Bits)
> > + 0 0 10
> > + 0 1 12
> > + 1 0 14
> > + 1 1 16
> > +
> > + Note on SPI connections: The CS line on the AD2S1210 should
> > + hard-wired to logic low and the WR/FSYNC line on the AD2S1210
> > + should be connected to the SPI CSn output of the SPI controller.
> > +
> > + Datasheet:
> > +
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/
> > + ad2s1210.pdf
> > +
> > +properties:
> > + compatible:
> > + const: adi,ad2s1210
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + spi-max-frequency:
> > + maximum: 25000000
> > +
> > + spi-cpha: true
> > +
> > + avdd-supply:
> > + description:
> > + A 4.75 to 5.25 V regulator that powers the Analog Supply Voltage
> (AVDD)
> > + pin.
> > +
> > + dvdd-supply:
> > + description:
> > + A 4.75 to 5.25 V regulator that powers the Digital Supply Voltage (DVDD)
> > + pin.
> > +
> > + vdrive-supply:
> > + description:
> > + A 2.3 to 5.25 V regulator that powers the Logic Power Supply Input
> > + (VDrive) pin.
> > +
> > + clocks:
> > + maxItems: 1
> > + description: External oscillator clock (CLKIN).
> > +
> > + reset-gpios:
> > + description:
> > + GPIO connected to the /RESET pin. As the line needs to be low for the
> > + reset to be active, it should be configured as GPIO_ACTIVE_LOW.
> > + maxItems: 1
> > +
> > + sample-gpios:
> > + description:
> > + GPIO connected to the /SAMPLE pin. As the line needs to be low to
> trigger
> > + a sample, it should be configured as GPIO_ACTIVE_LOW.
> > + maxItems: 1
> > +
> > + mode-gpios:
> > + description:
> > + GPIO lines connected to the A0 and A1 pins. These pins select the data
> > + transfer mode.
> > + minItems: 2
> > + maxItems: 2
> > +
> > + resolution-gpios:
> > + description:
> > + GPIO lines connected to the RES0 and RES1 pins. These pins select the
> > + resolution of the digital output. If omitted, it is assumed that the
> > + RES0 and RES1 pins are hard-wired to match the assigned-resolution-bits
> > + property.
> > + minItems: 2
> > + maxItems: 2
> > +
> > + fault-gpios:
> > + description:
> > + GPIO lines connected to the LOT and DOS pins. These pins combined
> indicate
> > + the type of fault present, if any. As these pins a pulled low to indicate
> > + a fault condition, they should be configured as GPIO_ACTIVE_LOW.
> > + minItems: 2
> > + maxItems: 2
> > +
> > + adi,fixed-mode:
> > + description:
> > + This is used to indicate the selected mode if A0 and A1 are hard-wired
> > + instead of connected to GPIOS (i.e. mode-gpios is omitted).
> > + $ref: /schemas/types.yaml#/definitions/string
> > + enum: [config, velocity, position]
> > +
> > + assigned-resolution-bits:
> > + description:
> > + Resolution of the digital output required by the application. This
> > + determines the precision of the angle and/or the maximum speed that
> can
> > + be measured. If resolution-gpios is omitted, it is assumed that RES0 and
> > + RES1 are hard-wired to match this value.
> > + enum: [10, 12, 14, 16]
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - spi-cpha
> > + - avdd-supply
> > + - dvdd-supply
> > + - vdrive-supply
> > + - clocks
> > + - sample-gpios
> > + - assigned-resolution-bits
> > +
> > +oneOf:
> > + - required:
> > + - mode-gpios
> > + - required:
> > + - adi,fixed-mode
> > +
> > +allOf:
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + #include <dt-bindings/gpio/gpio.h>
> > +
> > + spi {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + resolver@0 {
> > + compatible = "adi,ad2s1210";
> > + reg = <0>;
> > + spi-max-frequency = <20000000>;
> > + spi-cpha;
> > + avdd-supply = <&avdd_regulator>;
> > + dvdd-supply = <&dvdd_regulator>;
> > + vdrive-supply = <&vdrive_regulator>;
> > + clocks = <&ext_osc>;
> > + sample-gpios = <&gpio0 90 GPIO_ACTIVE_LOW>;
> > + mode-gpios = <&gpio0 86 0>, <&gpio0 87 0>;
> > + resolution-gpios = <&gpio0 88 0>, <&gpio0 89 0>;
> > + assigned-resolution-bits = <16>;
> > + };
> > + };
> >
On Mon, 2 Oct 2023 11:58:17 -0500
David Lechner <[email protected]> wrote:
> On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Fri, 29 Sep 2023 12:23:31 -0500
> > David Lechner <[email protected]> wrote:
> >
> > > From: David Lechner <[email protected]>
> > >
> > > From: David Lechner <[email protected]>
> > >
> > > When reading the position and velocity on the AD2S1210, there is also a
> > > 3rd byte following the two data bytes that contains the fault flag bits.
> > > This patch adds support for reading this byte and generating events when
> > > faults occur.
> > >
> > > The faults are mapped to various channels and event types in order to
> > > have a unique event for each fault.
> > >
> > > Signed-off-by: David Lechner <[email protected]>
> >
> > Use of x and y modifiers is a little odd. What was your reasoning?
> > Was it just that there was a X_OR_Y modifier? If so, don't use that!
> > It seemed like a good idea at the time, but it's not nice to deal with
> > and requires a channel with that modifier to hang the controls off
> > + make sure userspace expects that event code.
>
>
> Regarding the point about "requires a channel with that modifier to
> hang the controls off...". Although that comment was about modifiers,
> does it also apply in general.
>
> There are several fault events that don't have any configurable
> parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds
> max tracking rate_. So there won't be any attributes that contain the
> event specification for those (e.g. no `events/in_angl0_*`
> attributes). It sounds like this would be a problem as well?
It's fine to have a channel that doesn't have controls or the ability
to be read.
We do have history of doing what you have here (a couple
of accelerometers do it) but it's esoteric and rather hard for userspace
to comprehend so I'd rather not introduce it for other types of devices.
I think we should go with the most flexible option of allowing
events to trigger when they 'may be true' to incorporate this case.
Unfortunately I can't see another option that would scale to all the
random combinations of events that might occur. There are all sorts
of extensions we could make to the event descriptions, but only at the
cost of breaking backwards compatibility and simplicity.
SWith hindsight the whole IIO_MOD_X_OR_Y_OR_Z mess was a design error :(
We can teach userspace code about that quirk for accelerations where
the one that would be hard to handle is the AND case used for
freefall detectors (you detect that the signal magnitude is near 0 for
all axes). I can't think of another option for that one other than
the weird modifier (unlike this case)
>
> Should we consider a IIO_EV_INFO_LABEL so that we can have some sort
> of attribute (namely `events/<dir>_<channel spec>_label`) so that
> userspace can enumerate expected events for non-configurable events?
Probably needs something similar to channel labelling, so a separate
callback given we don't handle strings, but sure something like this
would be useful and provide 'hints' along the lines of what the
datasheet calls a particular event. Not however for what event is sent
as such info should be apparent from the event naming.
Jonathan
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
Since we never call spi_get_drvdata(), we can remove spi_set_drvdata().
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* This is a new patch split out from "staging: iio: resolver: ad2s1210:
fix probe"
drivers/staging/iio/resolver/ad2s1210.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index b5e071d7c5fd..28015322f562 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -659,8 +659,6 @@ static int ad2s1210_probe(struct spi_device *spi)
return -ENOMEM;
st = iio_priv(indio_dev);
- spi_set_drvdata(spi, indio_dev);
-
mutex_init(&st->lock);
st->sdev = spi;
st->hysteresis = true;
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
This add an implementation of debugfs_reg_access for the AD2S1210
driver.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes: None
drivers/staging/iio/resolver/ad2s1210.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 0663a51d04ad..31415fbb6384 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -614,9 +614,29 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
return ret;
}
+static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
+ unsigned int reg, unsigned int writeval,
+ unsigned int *readval)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&st->lock);
+
+ if (readval)
+ ret = regmap_read(st->regmap, reg, readval);
+ else
+ ret = regmap_write(st->regmap, reg, writeval);
+
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
static const struct iio_info ad2s1210_info = {
.read_raw = ad2s1210_read_raw,
.attrs = &ad2s1210_attribute_group,
+ .debugfs_reg_access = &ad2s1210_debugfs_reg_access,
};
static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
The AD2S1210 has a programmable threshold for the degradation of signal
(DOS) overrange fault. This fault is triggered when either the sine or
cosine input rises the threshold voltage.
This patch converts the custom device DOS overrange threshold attribute
to an event rising edge threshold attribute on the monitor signal
channel.
The attribute now uses millivolts instead of the raw register value in
accordance with the IIO ABI.
Emitting the event will be implemented in a later patch.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes: This is a new patch in v3
drivers/staging/iio/resolver/ad2s1210.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 7abbc184c351..66def9f1dd1b 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -743,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
static IIO_DEVICE_ATTR(fault, 0644,
ad2s1210_show_fault, ad2s1210_clear_fault, 0);
-static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644,
- ad2s1210_show_reg, ad2s1210_store_reg,
- AD2S1210_REG_DOS_OVR_THRD);
static IIO_DEVICE_ATTR(dos_mis_thrd, 0644,
ad2s1210_show_reg, ad2s1210_store_reg,
AD2S1210_REG_DOS_MIS_THRD);
@@ -787,6 +784,13 @@ static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
/* Loss of signal threshold. */
.mask_separate = BIT(IIO_EV_INFO_VALUE),
},
+ {
+ /* Sine/cosine DOS overrange fault.*/
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ /* Degredation of signal overrange threshold. */
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
};
static const struct iio_chan_spec ad2s1210_channels[] = {
@@ -860,7 +864,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
static struct attribute *ad2s1210_attributes[] = {
&iio_dev_attr_fault.dev_attr.attr,
- &iio_dev_attr_dos_ovr_thrd.dev_attr.attr,
&iio_dev_attr_dos_mis_thrd.dev_attr.attr,
&iio_dev_attr_dos_rst_max_thrd.dev_attr.attr,
&iio_dev_attr_dos_rst_min_thrd.dev_attr.attr,
@@ -899,12 +902,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available,
__stringify(PHASE_360_DEG_TO_RAD_INT) "."
__stringify(PHASE_360_DEG_TO_RAD_MICRO));
IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR);
+IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR);
IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
static struct attribute *ad2s1210_event_attributes[] = {
&iio_const_attr_in_phase0_mag_value_available.dev_attr.attr,
&iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr,
+ &iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr,
&iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr,
&iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr,
NULL,
@@ -963,6 +968,9 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
return ad2s1210_get_voltage_threshold(st,
AD2S1210_REG_LOS_THRD, val);
+ if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_RISING)
+ return ad2s1210_get_voltage_threshold(st,
+ AD2S1210_REG_DOS_OVR_THRD, val);
return -EINVAL;
case IIO_PHASE:
return ad2s1210_get_phase_lock_range(st, val, val2);
@@ -996,6 +1004,9 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
return ad2s1210_set_voltage_threshold(st,
AD2S1210_REG_LOS_THRD, val);
+ if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_RISING)
+ return ad2s1210_set_voltage_threshold(st,
+ AD2S1210_REG_DOS_OVR_THRD, val);
return -EINVAL;
case IIO_PHASE:
return ad2s1210_set_phase_lock_range(st, val, val2);
--
2.42.0
From: David Lechner <[email protected]>
From: David Lechner <[email protected]>
Selecting the resolution was implemented as the `bits` sysfs attribute.
However, the selection of the resolution depends on how the hardware
is wired and the specific application, so this is rather a job for
devicetree to describe.
A new devicetree property `adi,resolution` to specify the resolution
required for each chip is added and the `bits` sysfs attribute is
removed.
Since the resolution is now supplied by a devicetree property, the
resolution-gpios are now optional and we can allow for the case where
the resolution pins on the AD2S1210 are hard-wired instead of requiring
them to be connected to gpios.
Signed-off-by: David Lechner <[email protected]>
---
v3 changes:
* Fixed multiline comment style.
drivers/staging/iio/resolver/ad2s1210.c | 136 ++++++++++++++------------------
1 file changed, 61 insertions(+), 75 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 6accb9e3db46..a0a426d0af19 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -63,6 +63,13 @@ enum ad2s1210_mode {
MOD_CONFIG = 0b11,
};
+enum ad2s1210_resolution {
+ AD2S1210_RES_10 = 0b00,
+ AD2S1210_RES_12 = 0b01,
+ AD2S1210_RES_14 = 0b10,
+ AD2S1210_RES_16 = 0b11,
+};
+
struct ad2s1210_state {
struct mutex lock;
struct spi_device *sdev;
@@ -70,13 +77,12 @@ struct ad2s1210_state {
struct gpio_desc *sample_gpio;
/** GPIO pins connected to A0 and A1 lines. */
struct gpio_descs *mode_gpios;
- /** GPIO pins connected to RES0 and RES1 lines. */
- struct gpio_descs *resolution_gpios;
/** Used to access config registers. */
struct regmap *regmap;
/** The external oscillator frequency in Hz. */
unsigned long clkin_hz;
- u8 resolution;
+ /** The selected resolution */
+ enum ad2s1210_resolution resolution;
/** For reading raw sample value via SPI. */
__be16 sample __aligned(IIO_DMA_MINALIGN);
/** SPI transmit buffer. */
@@ -212,62 +218,6 @@ static int ad2s1210_reinit_excitation_frequency(struct ad2s1210_state *st,
return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0);
}
-static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
- u8 resolution)
-{
- struct gpio_descs *gpios = st->resolution_gpios;
- DECLARE_BITMAP(bitmap, 2);
-
- bitmap[0] = (resolution - 10) >> 1;
-
- return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info,
- bitmap);
-}
-
-static ssize_t ad2s1210_show_resolution(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
-
- return sprintf(buf, "%d\n", st->resolution);
-}
-
-static ssize_t ad2s1210_store_resolution(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t len)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- unsigned char data;
- unsigned char udata;
- int ret;
-
- ret = kstrtou8(buf, 10, &udata);
- if (ret || udata < 10 || udata > 16) {
- dev_err(dev, "ad2s1210: resolution out of range\n");
- return -EINVAL;
- }
-
- data = (udata - 10) >> 1;
-
- mutex_lock(&st->lock);
- ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
- AD2S1210_SET_RES, data);
- if (ret < 0)
- goto error_ret;
-
- ret = ad2s1210_set_resolution_gpios(st, udata);
- if (ret < 0)
- goto error_ret;
-
- st->resolution = udata;
- ret = len;
-
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
-}
-
/* read the fault register since last sample */
static ssize_t ad2s1210_show_fault(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -572,8 +522,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
}
}
-static IIO_DEVICE_ATTR(bits, 0644,
- ad2s1210_show_resolution, ad2s1210_store_resolution, 0);
static IIO_DEVICE_ATTR(fault, 0644,
ad2s1210_show_fault, ad2s1210_clear_fault, 0);
@@ -628,7 +576,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
};
static struct attribute *ad2s1210_attributes[] = {
- &iio_dev_attr_bits.dev_attr.attr,
&iio_dev_attr_fault.dev_attr.attr,
&iio_dev_attr_los_thrd.dev_attr.attr,
&iio_dev_attr_dos_ovr_thrd.dev_attr.attr,
@@ -650,15 +597,12 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
int ret;
mutex_lock(&st->lock);
- ret = ad2s1210_set_resolution_gpios(st, st->resolution);
- if (ret < 0)
- return ret;
/* Use default config register value plus resolution from devicetree. */
data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1);
data |= FIELD_PREP(AD2S1210_ENABLE_HYSTERESIS, 1);
data |= FIELD_PREP(AD2S1210_SET_ENRES, 0x3);
- data |= FIELD_PREP(AD2S1210_SET_RES, (st->resolution - 10) >> 1);
+ data |= FIELD_PREP(AD2S1210_SET_RES, st->resolution);
ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
if (ret < 0)
@@ -698,6 +642,26 @@ static const struct iio_info ad2s1210_info = {
.debugfs_reg_access = &ad2s1210_debugfs_reg_access,
};
+static int ad2s1210_setup_properties(struct ad2s1210_state *st)
+{
+ struct device *dev = &st->sdev->dev;
+ u32 val;
+ int ret;
+
+ ret = device_property_read_u32(dev, "assigned-resolution-bits", &val);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to read assigned-resolution-bits property\n");
+
+ if (val < 10 || val > 16)
+ return dev_err_probe(dev, -EINVAL,
+ "resolution out of range: %u\n", val);
+
+ st->resolution = (val - 10) >> 1;
+
+ return 0;
+}
+
static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
{
struct device *dev = &st->sdev->dev;
@@ -719,6 +683,9 @@ static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
{
struct device *dev = &st->sdev->dev;
+ struct gpio_descs *resolution_gpios;
+ DECLARE_BITMAP(bitmap, 2);
+ int ret;
/* should not be sampling on startup */
st->sample_gpio = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW);
@@ -736,16 +703,32 @@ static int ad2s1210_setup_gpios(struct ad2s1210_state *st)
return dev_err_probe(dev, -EINVAL,
"requires exactly 2 mode-gpios\n");
- /* both pins high means that we start with 16-bit resolution */
- st->resolution_gpios = devm_gpiod_get_array(dev, "resolution",
- GPIOD_OUT_HIGH);
- if (IS_ERR(st->resolution_gpios))
- return dev_err_probe(dev, PTR_ERR(st->resolution_gpios),
+ /*
+ * If resolution gpios are provided, they get set to the required
+ * resolution, otherwise it is assumed the RES0 and RES1 pins are
+ * hard-wired to match the resolution indicated in the devicetree.
+ */
+ resolution_gpios = devm_gpiod_get_array_optional(dev, "resolution",
+ GPIOD_ASIS);
+ if (IS_ERR(resolution_gpios))
+ return dev_err_probe(dev, PTR_ERR(resolution_gpios),
"failed to request resolution GPIOs\n");
- if (st->resolution_gpios->ndescs != 2)
- return dev_err_probe(dev, -EINVAL,
- "requires exactly 2 resolution-gpios\n");
+ if (resolution_gpios) {
+ if (resolution_gpios->ndescs != 2)
+ return dev_err_probe(dev, -EINVAL,
+ "requires exactly 2 resolution-gpios\n");
+
+ bitmap[0] = st->resolution;
+
+ ret = gpiod_set_array_value(resolution_gpios->ndescs,
+ resolution_gpios->desc,
+ resolution_gpios->info,
+ bitmap);
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+ "failed to set resolution gpios\n");
+ }
return 0;
}
@@ -809,7 +792,10 @@ static int ad2s1210_probe(struct spi_device *spi)
mutex_init(&st->lock);
st->sdev = spi;
- st->resolution = 12;
+
+ ret = ad2s1210_setup_properties(st);
+ if (ret < 0)
+ return ret;
ret = ad2s1210_setup_clocks(st);
if (ret < 0)
--
2.42.0