* [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:38 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 02/17] staging: iio: resolver: ad2s1210: implement hysteresis as channel attr David Lechner
` (15 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
When reading registers on the AD2S1210 chip, we have to supply a "dummy"
address for the second SPI tx byte so that we don't accidentally write
to a register. This register will be read and the value discarded on the
next regmap read or write call.
Reading the fault register has a side-effect of clearing the faults
so we should not use this register for the dummy read.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: New patch
(this probably should have been done before "staging: iio: resolver:
ad2s1210: use regmap for config registers" but was overlooked until now)
drivers/staging/iio/resolver/ad2s1210.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 67d8af0dd7ae..8fbde9517fe9 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -166,9 +166,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
st->tx[0] = reg;
/*
* Must be valid register address here otherwise this could write data.
- * It doesn't matter which one.
+ * It doesn't matter which one as long as reading doesn't have side-
+ * effects.
*/
- st->tx[1] = AD2S1210_REG_FAULT;
+ st->tx[1] = AD2S1210_REG_CONTROL;
ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
if (ret < 0)
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read
2023-10-06 0:50 ` [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read David Lechner
@ 2023-10-10 15:38 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:38 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:18 -0500
David Lechner <dlechner@baylibre.com> wrote:
> When reading registers on the AD2S1210 chip, we have to supply a "dummy"
> address for the second SPI tx byte so that we don't accidentally write
> to a register. This register will be read and the value discarded on the
> next regmap read or write call.
>
> Reading the fault register has a side-effect of clearing the faults
> so we should not use this register for the dummy read.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
ouch.
Applied to the togreg branch of iio.git and pushed out as testing for 0day
to take a look at it.
Thanks,
Jonathan
> ---
>
> v4 changes: New patch
>
> (this probably should have been done before "staging: iio: resolver:
> ad2s1210: use regmap for config registers" but was overlooked until now)
>
> drivers/staging/iio/resolver/ad2s1210.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 67d8af0dd7ae..8fbde9517fe9 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -166,9 +166,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg,
> st->tx[0] = reg;
> /*
> * Must be valid register address here otherwise this could write data.
> - * It doesn't matter which one.
> + * It doesn't matter which one as long as reading doesn't have side-
> + * effects.
> */
> - st->tx[1] = AD2S1210_REG_FAULT;
> + st->tx[1] = AD2S1210_REG_CONTROL;
>
> ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers));
> if (ret < 0)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 02/17] staging: iio: resolver: ad2s1210: implement hysteresis as channel attr
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
2023-10-06 0:50 ` [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:40 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 03/17] staging: iio: resolver: ad2s1210: convert fexcit to channel attribute David Lechner
` (14 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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 <dlechner@baylibre.com>
---
v4 changes:
* Fixed hysteresis raw values when st->resolution != 16.
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 | 91 +++++++++++++++++++++++++++++++--
1 file changed, 88 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 8fbde9517fe9..af063eb25e9c 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -76,7 +76,8 @@ struct ad2s1210_state {
struct regmap *regmap;
/** The external oscillator frequency in Hz. */
unsigned long clkin_hz;
- bool hysteresis;
+ /** Available raw hysteresis values based on resolution. */
+ int hysteresis_available[2];
u8 resolution;
/** For reading raw sample value via SPI. */
__be16 sample __aligned(IIO_DMA_MINALIGN);
@@ -311,6 +312,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
goto error_ret;
st->resolution = udata;
+ st->hysteresis_available[1] = 1 << (16 - st->resolution);
ret = len;
error_ret:
@@ -447,6 +449,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 << (16 - st->resolution);
+ 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) */
@@ -479,7 +510,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)
+{
+ struct ad2s1210_state *st = iio_priv(indio_dev);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_HYSTERESIS:
+ switch (chan->type) {
+ case IIO_ANGL:
+ *vals = st->hysteresis_available;
+ *type = IIO_VAL_INT;
+ *length = ARRAY_SIZE(st->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;
}
@@ -520,7 +599,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,
@@ -596,6 +678,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,
};
@@ -711,8 +795,9 @@ static int ad2s1210_probe(struct spi_device *spi)
mutex_init(&st->lock);
st->sdev = spi;
- st->hysteresis = true;
st->resolution = 12;
+ st->hysteresis_available[0] = 0;
+ st->hysteresis_available[1] = 1 << (16 - st->resolution);
ret = ad2s1210_setup_clocks(st);
if (ret < 0)
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 02/17] staging: iio: resolver: ad2s1210: implement hysteresis as channel attr
2023-10-06 0:50 ` [PATCH v4 02/17] staging: iio: resolver: ad2s1210: implement hysteresis as channel attr David Lechner
@ 2023-10-10 15:40 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:40 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:19 -0500
David Lechner <dlechner@baylibre.com> wrote:
> 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 <dlechner@baylibre.com>
Applied
> ---
>
> v4 changes:
> * Fixed hysteresis raw values when st->resolution != 16.
>
> 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 | 91 +++++++++++++++++++++++++++++++--
> 1 file changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 8fbde9517fe9..af063eb25e9c 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -76,7 +76,8 @@ struct ad2s1210_state {
> struct regmap *regmap;
> /** The external oscillator frequency in Hz. */
> unsigned long clkin_hz;
> - bool hysteresis;
> + /** Available raw hysteresis values based on resolution. */
> + int hysteresis_available[2];
> u8 resolution;
> /** For reading raw sample value via SPI. */
> __be16 sample __aligned(IIO_DMA_MINALIGN);
> @@ -311,6 +312,7 @@ static ssize_t ad2s1210_store_resolution(struct device *dev,
> goto error_ret;
>
> st->resolution = udata;
> + st->hysteresis_available[1] = 1 << (16 - st->resolution);
> ret = len;
>
> error_ret:
> @@ -447,6 +449,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 << (16 - st->resolution);
> + 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) */
> @@ -479,7 +510,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)
> +{
> + struct ad2s1210_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HYSTERESIS:
> + switch (chan->type) {
> + case IIO_ANGL:
> + *vals = st->hysteresis_available;
> + *type = IIO_VAL_INT;
> + *length = ARRAY_SIZE(st->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;
> }
> @@ -520,7 +599,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,
> @@ -596,6 +678,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,
> };
> @@ -711,8 +795,9 @@ static int ad2s1210_probe(struct spi_device *spi)
>
> mutex_init(&st->lock);
> st->sdev = spi;
> - st->hysteresis = true;
> st->resolution = 12;
> + st->hysteresis_available[0] = 0;
> + st->hysteresis_available[1] = 1 << (16 - st->resolution);
>
> ret = ad2s1210_setup_clocks(st);
> if (ret < 0)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 03/17] staging: iio: resolver: ad2s1210: convert fexcit to channel attribute
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
2023-10-06 0:50 ` [PATCH v4 01/17] staging: iio: resolver: ad2s1210: do not use fault register for dummy read David Lechner
2023-10-06 0:50 ` [PATCH v4 02/17] staging: iio: resolver: ad2s1210: implement hysteresis as channel attr David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:41 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 04/17] staging: iio: resolver: ad2s1210: convert resolution to devicetree property David Lechner
` (13 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
The ad2s1210 driver has a device-specific attribute `fexcit` for setting
the frequency of the excitation output. This converts it to a channel in
order to use standard IIO ABI.
The excitation frequency is an analog output that generates a sine wave.
Only the frequency is configurable. According to the datasheet, the
specified range of the excitation frequency is from 2 kHz to 20 kHz and
can be set in increments of 250 Hz.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: None (rebased)
v3 changes:
* This is a new patch in v3 instead of "iio: resolver: ad2s1210: rename fexcit
attribute"
drivers/staging/iio/resolver/ad2s1210.c | 123 ++++++++++++++++++--------------
1 file changed, 71 insertions(+), 52 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index af063eb25e9c..0c7772725330 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -227,54 +227,6 @@ static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
bitmap);
}
-static ssize_t ad2s1210_show_fexcit(struct device *dev,
- struct device_attribute *attr,
- char *buf)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- unsigned int value;
- u16 fexcit;
- int ret;
-
- mutex_lock(&st->lock);
- ret = regmap_read(st->regmap, AD2S1210_REG_EXCIT_FREQ, &value);
- if (ret < 0)
- goto error_ret;
-
- fexcit = value * st->clkin_hz / (1 << 15);
-
- ret = sprintf(buf, "%u\n", fexcit);
-
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
-}
-
-static ssize_t ad2s1210_store_fexcit(struct device *dev,
- struct device_attribute *attr,
- const char *buf, size_t len)
-{
- struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
- u16 fexcit;
- int ret;
-
- ret = kstrtou16(buf, 10, &fexcit);
- if (ret < 0 || fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT)
- return -EINVAL;
-
- mutex_lock(&st->lock);
- ret = ad2s1210_reinit_excitation_frequency(st, fexcit);
- if (ret < 0)
- goto error_ret;
-
- ret = len;
-
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret;
-}
-
static ssize_t ad2s1210_show_resolution(struct device *dev,
struct device_attribute *attr,
char *buf)
@@ -478,6 +430,38 @@ static int ad2s1210_set_hysteresis(struct ad2s1210_state *st, int val)
return ret;
}
+static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
+{
+ unsigned int reg_val;
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_read(st->regmap, AD2S1210_REG_EXCIT_FREQ, ®_val);
+ if (ret < 0)
+ goto error_ret;
+
+ *val = reg_val * st->clkin_hz / (1 << 15);
+ ret = IIO_VAL_INT;
+
+error_ret:
+ mutex_unlock(&st->lock);
+ return ret;
+}
+
+static int ad2s1210_set_excitation_frequency(struct ad2s1210_state *st, int val)
+{
+ int ret;
+
+ if (val < AD2S1210_MIN_EXCIT || val > AD2S1210_MAX_EXCIT)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+ ret = ad2s1210_reinit_excitation_frequency(st, val);
+ 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) */
@@ -510,6 +494,13 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_CHAN_INFO_FREQUENCY:
+ switch (chan->type) {
+ case IIO_ALTVOLTAGE:
+ return ad2s1210_get_excitation_frequency(st, val);
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_HYSTERESIS:
switch (chan->type) {
case IIO_ANGL:
@@ -527,9 +518,24 @@ static int ad2s1210_read_avail(struct iio_dev *indio_dev,
const int **vals, int *type,
int *length, long mask)
{
+ static const int excitation_frequency_available[] = {
+ AD2S1210_MIN_EXCIT,
+ 250, /* step */
+ AD2S1210_MAX_EXCIT,
+ };
+
struct ad2s1210_state *st = iio_priv(indio_dev);
switch (mask) {
+ case IIO_CHAN_INFO_FREQUENCY:
+ switch (chan->type) {
+ case IIO_ALTVOLTAGE:
+ *type = IIO_VAL_INT;
+ *vals = excitation_frequency_available;
+ return IIO_AVAIL_RANGE;
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_HYSTERESIS:
switch (chan->type) {
case IIO_ANGL:
@@ -552,6 +558,13 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
struct ad2s1210_state *st = iio_priv(indio_dev);
switch (mask) {
+ case IIO_CHAN_INFO_FREQUENCY:
+ switch (chan->type) {
+ case IIO_ALTVOLTAGE:
+ return ad2s1210_set_excitation_frequency(st, val);
+ default:
+ return -EINVAL;
+ }
case IIO_CHAN_INFO_HYSTERESIS:
switch (chan->type) {
case IIO_ANGL:
@@ -564,8 +577,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
}
}
-static IIO_DEVICE_ATTR(fexcit, 0644,
- ad2s1210_show_fexcit, ad2s1210_store_fexcit, 0);
static IIO_DEVICE_ATTR(bits, 0644,
ad2s1210_show_resolution, ad2s1210_store_resolution, 0);
static IIO_DEVICE_ATTR(fault, 0644,
@@ -609,11 +620,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.channel = 0,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
BIT(IIO_CHAN_INFO_SCALE),
- }
+ }, {
+ /* excitation frequency output */
+ .type = IIO_ALTVOLTAGE,
+ .indexed = 1,
+ .channel = 0,
+ .output = 1,
+ .scan_index = -1,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
+ .info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
+ },
};
static struct attribute *ad2s1210_attributes[] = {
- &iio_dev_attr_fexcit.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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 03/17] staging: iio: resolver: ad2s1210: convert fexcit to channel attribute
2023-10-06 0:50 ` [PATCH v4 03/17] staging: iio: resolver: ad2s1210: convert fexcit to channel attribute David Lechner
@ 2023-10-10 15:41 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:41 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:20 -0500
David Lechner <dlechner@baylibre.com> wrote:
> The ad2s1210 driver has a device-specific attribute `fexcit` for setting
> the frequency of the excitation output. This converts it to a channel in
> order to use standard IIO ABI.
>
> The excitation frequency is an analog output that generates a sine wave.
> Only the frequency is configurable. According to the datasheet, the
> specified range of the excitation frequency is from 2 kHz to 20 kHz and
> can be set in increments of 250 Hz.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied
> ---
>
> v4 changes: None (rebased)
>
> v3 changes:
> * This is a new patch in v3 instead of "iio: resolver: ad2s1210: rename fexcit
> attribute"
>
>
> drivers/staging/iio/resolver/ad2s1210.c | 123 ++++++++++++++++++--------------
> 1 file changed, 71 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index af063eb25e9c..0c7772725330 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -227,54 +227,6 @@ static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st,
> bitmap);
> }
>
> -static ssize_t ad2s1210_show_fexcit(struct device *dev,
> - struct device_attribute *attr,
> - char *buf)
> -{
> - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> - unsigned int value;
> - u16 fexcit;
> - int ret;
> -
> - mutex_lock(&st->lock);
> - ret = regmap_read(st->regmap, AD2S1210_REG_EXCIT_FREQ, &value);
> - if (ret < 0)
> - goto error_ret;
> -
> - fexcit = value * st->clkin_hz / (1 << 15);
> -
> - ret = sprintf(buf, "%u\n", fexcit);
> -
> -error_ret:
> - mutex_unlock(&st->lock);
> - return ret;
> -}
> -
> -static ssize_t ad2s1210_store_fexcit(struct device *dev,
> - struct device_attribute *attr,
> - const char *buf, size_t len)
> -{
> - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> - u16 fexcit;
> - int ret;
> -
> - ret = kstrtou16(buf, 10, &fexcit);
> - if (ret < 0 || fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT)
> - return -EINVAL;
> -
> - mutex_lock(&st->lock);
> - ret = ad2s1210_reinit_excitation_frequency(st, fexcit);
> - if (ret < 0)
> - goto error_ret;
> -
> - ret = len;
> -
> -error_ret:
> - mutex_unlock(&st->lock);
> -
> - return ret;
> -}
> -
> static ssize_t ad2s1210_show_resolution(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -478,6 +430,38 @@ static int ad2s1210_set_hysteresis(struct ad2s1210_state *st, int val)
> return ret;
> }
>
> +static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_read(st->regmap, AD2S1210_REG_EXCIT_FREQ, ®_val);
> + if (ret < 0)
> + goto error_ret;
> +
> + *val = reg_val * st->clkin_hz / (1 << 15);
> + ret = IIO_VAL_INT;
> +
> +error_ret:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int ad2s1210_set_excitation_frequency(struct ad2s1210_state *st, int val)
> +{
> + int ret;
> +
> + if (val < AD2S1210_MIN_EXCIT || val > AD2S1210_MAX_EXCIT)
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
> + ret = ad2s1210_reinit_excitation_frequency(st, val);
> + 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) */
> @@ -510,6 +494,13 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_CHAN_INFO_FREQUENCY:
> + switch (chan->type) {
> + case IIO_ALTVOLTAGE:
> + return ad2s1210_get_excitation_frequency(st, val);
> + default:
> + return -EINVAL;
> + }
> case IIO_CHAN_INFO_HYSTERESIS:
> switch (chan->type) {
> case IIO_ANGL:
> @@ -527,9 +518,24 @@ static int ad2s1210_read_avail(struct iio_dev *indio_dev,
> const int **vals, int *type,
> int *length, long mask)
> {
> + static const int excitation_frequency_available[] = {
> + AD2S1210_MIN_EXCIT,
> + 250, /* step */
> + AD2S1210_MAX_EXCIT,
> + };
> +
> struct ad2s1210_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> + switch (chan->type) {
> + case IIO_ALTVOLTAGE:
> + *type = IIO_VAL_INT;
> + *vals = excitation_frequency_available;
> + return IIO_AVAIL_RANGE;
> + default:
> + return -EINVAL;
> + }
> case IIO_CHAN_INFO_HYSTERESIS:
> switch (chan->type) {
> case IIO_ANGL:
> @@ -552,6 +558,13 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> struct ad2s1210_state *st = iio_priv(indio_dev);
>
> switch (mask) {
> + case IIO_CHAN_INFO_FREQUENCY:
> + switch (chan->type) {
> + case IIO_ALTVOLTAGE:
> + return ad2s1210_set_excitation_frequency(st, val);
> + default:
> + return -EINVAL;
> + }
> case IIO_CHAN_INFO_HYSTERESIS:
> switch (chan->type) {
> case IIO_ANGL:
> @@ -564,8 +577,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> -static IIO_DEVICE_ATTR(fexcit, 0644,
> - ad2s1210_show_fexcit, ad2s1210_store_fexcit, 0);
> static IIO_DEVICE_ATTR(bits, 0644,
> ad2s1210_show_resolution, ad2s1210_store_resolution, 0);
> static IIO_DEVICE_ATTR(fault, 0644,
> @@ -609,11 +620,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> .channel = 0,
> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> BIT(IIO_CHAN_INFO_SCALE),
> - }
> + }, {
> + /* excitation frequency output */
> + .type = IIO_ALTVOLTAGE,
> + .indexed = 1,
> + .channel = 0,
> + .output = 1,
> + .scan_index = -1,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
> + },
> };
>
> static struct attribute *ad2s1210_attributes[] = {
> - &iio_dev_attr_fexcit.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,
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 04/17] staging: iio: resolver: ad2s1210: convert resolution to devicetree property
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (2 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 03/17] staging: iio: resolver: ad2s1210: convert fexcit to channel attribute David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:43 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 05/17] staging: iio: resolver: ad2s1210: add phase lock range support David Lechner
` (12 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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 `assigned-resolution-bits` 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 <dlechner@baylibre.com>
---
v4 changes:
* Fixed devicetree property name in commit message.
* Reworked handling of hysteresis_available to handle
assigned-resolution-bits != 16.
v3 changes:
* Fixed multiline comment style.
drivers/staging/iio/resolver/ad2s1210.c | 150 +++++++++++++++-----------------
1 file changed, 71 insertions(+), 79 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 0c7772725330..66ef35fbb6fe 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,15 +77,14 @@ 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;
/** Available raw hysteresis values based on resolution. */
int hysteresis_available[2];
- 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. */
@@ -215,63 +221,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;
- st->hysteresis_available[1] = 1 << (16 - st->resolution);
- 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)
@@ -413,7 +362,7 @@ static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val)
if (ret < 0)
return ret;
- *val = ret << (16 - st->resolution);
+ *val = ret << (2 * (AD2S1210_RES_16 - st->resolution));
return IIO_VAL_INT;
}
@@ -577,8 +526,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);
@@ -633,7 +580,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,
@@ -655,15 +601,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)
- goto error_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)
@@ -703,6 +646,35 @@ 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;
+ /*
+ * These are values that correlate to the hysteresis bit in the Control
+ * register. 0 = disabled, 1 = enabled. When enabled, the actual
+ * hysteresis is +/- 1 LSB of the raw position value. Which bit is the
+ * LSB depends on the specified resolution.
+ */
+ st->hysteresis_available[0] = 0;
+ st->hysteresis_available[1] = 1 << (2 * (AD2S1210_RES_16 -
+ st->resolution));
+
+ return 0;
+}
+
static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
{
struct device *dev = &st->sdev->dev;
@@ -724,6 +696,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);
@@ -741,16 +716,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;
}
@@ -814,9 +805,10 @@ static int ad2s1210_probe(struct spi_device *spi)
mutex_init(&st->lock);
st->sdev = spi;
- st->resolution = 12;
- st->hysteresis_available[0] = 0;
- st->hysteresis_available[1] = 1 << (16 - st->resolution);
+
+ ret = ad2s1210_setup_properties(st);
+ if (ret < 0)
+ return ret;
ret = ad2s1210_setup_clocks(st);
if (ret < 0)
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 04/17] staging: iio: resolver: ad2s1210: convert resolution to devicetree property
2023-10-06 0:50 ` [PATCH v4 04/17] staging: iio: resolver: ad2s1210: convert resolution to devicetree property David Lechner
@ 2023-10-10 15:43 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:43 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:21 -0500
David Lechner <dlechner@baylibre.com> wrote:
> 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 `assigned-resolution-bits` 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 <dlechner@baylibre.com>
Applied
> ---
>
> v4 changes:
> * Fixed devicetree property name in commit message.
> * Reworked handling of hysteresis_available to handle
> assigned-resolution-bits != 16.
>
> v3 changes:
> * Fixed multiline comment style.
>
> drivers/staging/iio/resolver/ad2s1210.c | 150 +++++++++++++++-----------------
> 1 file changed, 71 insertions(+), 79 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 0c7772725330..66ef35fbb6fe 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,15 +77,14 @@ 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;
> /** Available raw hysteresis values based on resolution. */
> int hysteresis_available[2];
> - 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. */
> @@ -215,63 +221,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;
> - st->hysteresis_available[1] = 1 << (16 - st->resolution);
> - 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)
> @@ -413,7 +362,7 @@ static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val)
> if (ret < 0)
> return ret;
>
> - *val = ret << (16 - st->resolution);
> + *val = ret << (2 * (AD2S1210_RES_16 - st->resolution));
> return IIO_VAL_INT;
> }
>
> @@ -577,8 +526,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);
>
> @@ -633,7 +580,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,
> @@ -655,15 +601,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)
> - goto error_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)
> @@ -703,6 +646,35 @@ 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;
> + /*
> + * These are values that correlate to the hysteresis bit in the Control
> + * register. 0 = disabled, 1 = enabled. When enabled, the actual
> + * hysteresis is +/- 1 LSB of the raw position value. Which bit is the
> + * LSB depends on the specified resolution.
> + */
> + st->hysteresis_available[0] = 0;
> + st->hysteresis_available[1] = 1 << (2 * (AD2S1210_RES_16 -
> + st->resolution));
> +
> + return 0;
> +}
> +
> static int ad2s1210_setup_clocks(struct ad2s1210_state *st)
> {
> struct device *dev = &st->sdev->dev;
> @@ -724,6 +696,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);
> @@ -741,16 +716,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;
> }
> @@ -814,9 +805,10 @@ static int ad2s1210_probe(struct spi_device *spi)
>
> mutex_init(&st->lock);
> st->sdev = spi;
> - st->resolution = 12;
> - st->hysteresis_available[0] = 0;
> - st->hysteresis_available[1] = 1 << (16 - st->resolution);
> +
> + ret = ad2s1210_setup_properties(st);
> + if (ret < 0)
> + return ret;
>
> ret = ad2s1210_setup_clocks(st);
> if (ret < 0)
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 05/17] staging: iio: resolver: ad2s1210: add phase lock range support
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (3 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 04/17] staging: iio: resolver: ad2s1210: convert resolution to devicetree property David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:46 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 06/17] staging: iio: resolver: ad2s1210: add triggered buffer support David Lechner
` (11 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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 phase0_mag_rising 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 <dlechner@baylibre.com>
---
v4 changes:
* Changed event direction from none to rising.
* Fixed missing static qualifier on attribute definition.
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 66ef35fbb6fe..83f6ac890dbc 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,
@@ -379,6 +386,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;
@@ -551,6 +606,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_RISING,
+ /* Phase lock range. */
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+};
+
static const struct iio_chan_spec ad2s1210_channels[] = {
{
.type = IIO_ANGL,
@@ -567,6 +632,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,
@@ -595,6 +668,21 @@ static const struct attribute_group ad2s1210_attribute_group = {
.attrs = ad2s1210_attributes,
};
+static IIO_CONST_ATTR(in_phase0_mag_rising_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_rising_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;
@@ -619,6 +707,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)
@@ -639,10 +761,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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 05/17] staging: iio: resolver: ad2s1210: add phase lock range support
2023-10-06 0:50 ` [PATCH v4 05/17] staging: iio: resolver: ad2s1210: add phase lock range support David Lechner
@ 2023-10-10 15:46 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:46 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:22 -0500
David Lechner <dlechner@baylibre.com> wrote:
> 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 phase0_mag_rising 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 <dlechner@baylibre.com>
Whilst I'm not feeling totally happy with some of the error condition
handling we have here, I think it's the best we can do without inventing
a whole knew path for error records. Whilst I have some thoughts on that
and using the tracepoint approach used for RAS event handling for servers
etc and the heavier weight userspace software that entails, we can always
bolt that on top later and it doesn't solve the control element anyway..
So applied,
Thanks,
Jonathan
> ---
>
> v4 changes:
> * Changed event direction from none to rising.
> * Fixed missing static qualifier on attribute definition.
>
> 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 66ef35fbb6fe..83f6ac890dbc 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,
> @@ -379,6 +386,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;
> @@ -551,6 +606,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_RISING,
> + /* Phase lock range. */
> + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> + },
> +};
> +
> static const struct iio_chan_spec ad2s1210_channels[] = {
> {
> .type = IIO_ANGL,
> @@ -567,6 +632,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,
> @@ -595,6 +668,21 @@ static const struct attribute_group ad2s1210_attribute_group = {
> .attrs = ad2s1210_attributes,
> };
>
> +static IIO_CONST_ATTR(in_phase0_mag_rising_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_rising_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;
> @@ -619,6 +707,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)
> @@ -639,10 +761,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,
> };
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 06/17] staging: iio: resolver: ad2s1210: add triggered buffer support
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (4 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 05/17] staging: iio: resolver: ad2s1210: add phase lock range support David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:47 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 07/17] staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs David Lechner
` (10 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
This adds support for triggered buffers to the AD2S1210 resolver driver.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: None
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 83f6ac890dbc..4d651a2d0f38 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"
@@ -94,6 +97,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. */
@@ -621,6 +630,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),
@@ -630,9 +646,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,
@@ -760,6 +785,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,
@@ -957,6 +1031,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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 07/17] staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (5 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 06/17] staging: iio: resolver: ad2s1210: add triggered buffer support David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:54 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 08/17] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr David Lechner
` (9 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
The AD2S1210 monitors the internal error signal (difference between
estimated angle and measured angle) to determine a loss of position
tracking (LOT) condition. When the error value exceeds a threshold, a
fault is triggered. This threshold is user-configurable.
This patch converts the custom lot_high_thrd and lot_low_thrd attributes
in the ad2s1210 driver to standard event attributes. This will allow
tooling to be able to expose these in a generic way.
Since the low threshold determines the hysteresis, it requires some
special handling to expose the difference between the high and low
register values as the hysteresis instead of exposing the low register
value directly.
The attributes also return the values in radians now as required by the
ABI.
Actually emitting the fault event will be done in a later patch.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes:
* Fixed missing static qualifier on attribute definition.
v3 changes: This is a new patch in v3
drivers/staging/iio/resolver/ad2s1210.c | 191 ++++++++++++++++++++++++++++++--
1 file changed, 183 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 4d651a2d0f38..12437f697f79 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -443,6 +443,123 @@ static int ad2s1210_set_phase_lock_range(struct ad2s1210_state *st,
return ret;
}
+/* map resolution to microradians/LSB for LOT registers */
+static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
+ 6184, /* 10-bit: ~0.35 deg/LSB, 45 deg max */
+ 2473, /* 12-bit: ~0.14 deg/LSB, 18 deg max */
+ 1237, /* 14-bit: ~0.07 deg/LSB, 9 deg max */
+ 1237, /* 16-bit: same as 14-bit */
+};
+
+static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
+ int *val, int *val2)
+{
+ unsigned int reg_val;
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, ®_val);
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ *val = 0;
+ *val2 = reg_val * ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad2s1210_set_lot_high_threshold(struct ad2s1210_state *st,
+ int val, int val2)
+{
+ unsigned int high_reg_val, low_reg_val, hysteresis;
+ int ret;
+
+ /* all valid values are between 0 and pi/4 radians */
+ if (val != 0)
+ return -EINVAL;
+
+ mutex_lock(&st->lock);
+ /*
+ * We need to read both high and low registers first so we can preserve
+ * the hysteresis.
+ */
+ ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
+ if (ret < 0)
+ goto error_ret;
+
+ ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
+ if (ret < 0)
+ goto error_ret;
+
+ hysteresis = high_reg_val - low_reg_val;
+ high_reg_val = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
+ low_reg_val = high_reg_val - hysteresis;
+
+ ret = regmap_write(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, high_reg_val);
+ if (ret < 0)
+ goto error_ret;
+
+ ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD, low_reg_val);
+
+error_ret:
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
+static int ad2s1210_get_lot_low_threshold(struct ad2s1210_state *st,
+ int *val, int *val2)
+{
+ unsigned int high_reg_val, low_reg_val;
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
+ if (ret < 0)
+ goto error_ret;
+
+ ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
+
+error_ret:
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ /* sysfs value is hysteresis rather than actual low value */
+ *val = 0;
+ *val2 = (high_reg_val - low_reg_val) *
+ ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
+ return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int ad2s1210_set_lot_low_threshold(struct ad2s1210_state *st,
+ int val, int val2)
+{
+ unsigned int reg_val, hysteresis;
+ int ret;
+
+ /* all valid values are between 0 and pi/4 radians */
+ if (val != 0)
+ return -EINVAL;
+
+ hysteresis = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
+
+ mutex_lock(&st->lock);
+ ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, ®_val);
+ if (ret < 0)
+ goto error_ret;
+
+ ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD,
+ reg_val - hysteresis);
+
+error_ret:
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
{
unsigned int reg_val;
@@ -608,12 +725,19 @@ static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
ad2s1210_show_reg, ad2s1210_store_reg,
AD2S1210_REG_DOS_RST_MIN_THRD);
-static IIO_DEVICE_ATTR(lot_high_thrd, 0644,
- ad2s1210_show_reg, ad2s1210_store_reg,
- AD2S1210_REG_LOT_HIGH_THRD);
-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_position_event_spec[] = {
+ {
+ /* Tracking error exceeds LOT threshold fault. */
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate =
+ /* Loss of tracking high threshold. */
+ BIT(IIO_EV_INFO_VALUE) |
+ /* Loss of tracking low threshold. */
+ BIT(IIO_EV_INFO_HYSTERESIS),
+ },
+};
static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
{
@@ -657,6 +781,15 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
BIT(IIO_CHAN_INFO_SCALE),
},
IIO_CHAN_SOFT_TIMESTAMP(2),
+ {
+ /* used to configure LOT thresholds and get tracking error */
+ .type = IIO_ANGL,
+ .indexed = 1,
+ .channel = 1,
+ .scan_index = -1,
+ .event_spec = ad2s1210_position_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_position_event_spec),
+ },
{
/* used to configure phase lock range and get phase lock error */
.type = IIO_PHASE,
@@ -684,8 +817,6 @@ static struct attribute *ad2s1210_attributes[] = {
&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,
- &iio_dev_attr_lot_high_thrd.dev_attr.attr,
- &iio_dev_attr_lot_low_thrd.dev_attr.attr,
NULL,
};
@@ -693,14 +824,40 @@ static const struct attribute_group ad2s1210_attribute_group = {
.attrs = ad2s1210_attributes,
};
+static ssize_t
+in_angl1_thresh_rising_value_available_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+ int step = ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
+
+ return sysfs_emit(buf, "[0 0.%06d 0.%06d]\n", step, step * 0x7F);
+}
+
+static ssize_t
+in_angl1_thresh_rising_hysteresis_available_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
+ int step = ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
+
+ return sysfs_emit(buf, "[0 0.%06d 0.%06d]\n", step, step * 0x7F);
+}
+
static IIO_CONST_ATTR(in_phase0_mag_rising_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 IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
+static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
static struct attribute *ad2s1210_event_attributes[] = {
&iio_const_attr_in_phase0_mag_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,
};
@@ -742,6 +899,15 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
struct ad2s1210_state *st = iio_priv(indio_dev);
switch (chan->type) {
+ case IIO_ANGL:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return ad2s1210_get_lot_high_threshold(st, val, val2);
+ case IIO_EV_INFO_HYSTERESIS:
+ return ad2s1210_get_lot_low_threshold(st, val, val2);
+ default:
+ return -EINVAL;
+ }
case IIO_PHASE:
return ad2s1210_get_phase_lock_range(st, val, val2);
default:
@@ -759,6 +925,15 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
struct ad2s1210_state *st = iio_priv(indio_dev);
switch (chan->type) {
+ case IIO_ANGL:
+ switch (info) {
+ case IIO_EV_INFO_VALUE:
+ return ad2s1210_set_lot_high_threshold(st, val, val2);
+ case IIO_EV_INFO_HYSTERESIS:
+ return ad2s1210_set_lot_low_threshold(st, val, val2);
+ default:
+ return -EINVAL;
+ }
case IIO_PHASE:
return ad2s1210_set_phase_lock_range(st, val, val2);
default:
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 07/17] staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs
2023-10-06 0:50 ` [PATCH v4 07/17] staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs David Lechner
@ 2023-10-10 15:54 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:54 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:24 -0500
David Lechner <dlechner@baylibre.com> wrote:
> The AD2S1210 monitors the internal error signal (difference between
> estimated angle and measured angle) to determine a loss of position
> tracking (LOT) condition. When the error value exceeds a threshold, a
> fault is triggered. This threshold is user-configurable.
>
> This patch converts the custom lot_high_thrd and lot_low_thrd attributes
> in the ad2s1210 driver to standard event attributes. This will allow
> tooling to be able to expose these in a generic way.
>
> Since the low threshold determines the hysteresis, it requires some
> special handling to expose the difference between the high and low
> register values as the hysteresis instead of exposing the low register
> value directly.
>
> The attributes also return the values in radians now as required by the
> ABI.
>
> Actually emitting the fault event will be done in a later patch.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
I noticed it first on next patch, but we are a bit vague on what the units
of event thresholds are - normally tracking an associated channel. That
gets tricky if there isn't an associated channel read facility.
I've applied this anyway, but we may want to think a bit more on that.
One approach is to add IIO_CHAN_INFO_SCALE and read_raw support for these
channels.
Not what you have here is valid without one of those because absence
of _scale, means _scale == 1 anyway so we are good with having these
in radians.
Jonathan
> ---
>
> v4 changes:
> * Fixed missing static qualifier on attribute definition.
>
> v3 changes: This is a new patch in v3
>
> drivers/staging/iio/resolver/ad2s1210.c | 191 ++++++++++++++++++++++++++++++--
> 1 file changed, 183 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 4d651a2d0f38..12437f697f79 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -443,6 +443,123 @@ static int ad2s1210_set_phase_lock_range(struct ad2s1210_state *st,
> return ret;
> }
>
> +/* map resolution to microradians/LSB for LOT registers */
> +static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
> + 6184, /* 10-bit: ~0.35 deg/LSB, 45 deg max */
> + 2473, /* 12-bit: ~0.14 deg/LSB, 18 deg max */
> + 1237, /* 14-bit: ~0.07 deg/LSB, 9 deg max */
> + 1237, /* 16-bit: same as 14-bit */
> +};
> +
> +static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
> + int *val, int *val2)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, ®_val);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = 0;
> + *val2 = reg_val * ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad2s1210_set_lot_high_threshold(struct ad2s1210_state *st,
> + int val, int val2)
> +{
> + unsigned int high_reg_val, low_reg_val, hysteresis;
> + int ret;
> +
> + /* all valid values are between 0 and pi/4 radians */
> + if (val != 0)
> + return -EINVAL;
> +
> + mutex_lock(&st->lock);
> + /*
> + * We need to read both high and low registers first so we can preserve
> + * the hysteresis.
> + */
> + ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
> + if (ret < 0)
> + goto error_ret;
> +
> + ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
> + if (ret < 0)
> + goto error_ret;
> +
> + hysteresis = high_reg_val - low_reg_val;
> + high_reg_val = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> + low_reg_val = high_reg_val - hysteresis;
> +
> + ret = regmap_write(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, high_reg_val);
> + if (ret < 0)
> + goto error_ret;
> +
> + ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD, low_reg_val);
> +
> +error_ret:
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int ad2s1210_get_lot_low_threshold(struct ad2s1210_state *st,
> + int *val, int *val2)
> +{
> + unsigned int high_reg_val, low_reg_val;
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
> + if (ret < 0)
> + goto error_ret;
> +
> + ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
> +
> +error_ret:
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + /* sysfs value is hysteresis rather than actual low value */
> + *val = 0;
> + *val2 = (high_reg_val - low_reg_val) *
> + ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int ad2s1210_set_lot_low_threshold(struct ad2s1210_state *st,
> + int val, int val2)
> +{
> + unsigned int reg_val, hysteresis;
> + int ret;
> +
> + /* all valid values are between 0 and pi/4 radians */
> + if (val != 0)
> + return -EINVAL;
> +
> + hysteresis = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +
> + mutex_lock(&st->lock);
> + ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, ®_val);
> + if (ret < 0)
> + goto error_ret;
> +
> + ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD,
> + reg_val - hysteresis);
> +
> +error_ret:
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
> {
> unsigned int reg_val;
> @@ -608,12 +725,19 @@ static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
> static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644,
> ad2s1210_show_reg, ad2s1210_store_reg,
> AD2S1210_REG_DOS_RST_MIN_THRD);
> -static IIO_DEVICE_ATTR(lot_high_thrd, 0644,
> - ad2s1210_show_reg, ad2s1210_store_reg,
> - AD2S1210_REG_LOT_HIGH_THRD);
> -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_position_event_spec[] = {
> + {
> + /* Tracking error exceeds LOT threshold fault. */
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate =
> + /* Loss of tracking high threshold. */
> + BIT(IIO_EV_INFO_VALUE) |
> + /* Loss of tracking low threshold. */
> + BIT(IIO_EV_INFO_HYSTERESIS),
> + },
> +};
>
> static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
> {
> @@ -657,6 +781,15 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> BIT(IIO_CHAN_INFO_SCALE),
> },
> IIO_CHAN_SOFT_TIMESTAMP(2),
> + {
> + /* used to configure LOT thresholds and get tracking error */
> + .type = IIO_ANGL,
> + .indexed = 1,
> + .channel = 1,
> + .scan_index = -1,
> + .event_spec = ad2s1210_position_event_spec,
> + .num_event_specs = ARRAY_SIZE(ad2s1210_position_event_spec),
> + },
> {
> /* used to configure phase lock range and get phase lock error */
> .type = IIO_PHASE,
> @@ -684,8 +817,6 @@ static struct attribute *ad2s1210_attributes[] = {
> &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,
> - &iio_dev_attr_lot_high_thrd.dev_attr.attr,
> - &iio_dev_attr_lot_low_thrd.dev_attr.attr,
> NULL,
> };
>
> @@ -693,14 +824,40 @@ static const struct attribute_group ad2s1210_attribute_group = {
> .attrs = ad2s1210_attributes,
> };
>
> +static ssize_t
> +in_angl1_thresh_rising_value_available_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> + int step = ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +
> + return sysfs_emit(buf, "[0 0.%06d 0.%06d]\n", step, step * 0x7F);
> +}
> +
> +static ssize_t
> +in_angl1_thresh_rising_hysteresis_available_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev));
> + int step = ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
> +
> + return sysfs_emit(buf, "[0 0.%06d 0.%06d]\n", step, step * 0x7F);
> +}
> +
> static IIO_CONST_ATTR(in_phase0_mag_rising_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 IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> +static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
>
> static struct attribute *ad2s1210_event_attributes[] = {
> &iio_const_attr_in_phase0_mag_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,
> };
>
> @@ -742,6 +899,15 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
> struct ad2s1210_state *st = iio_priv(indio_dev);
>
> switch (chan->type) {
> + case IIO_ANGL:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + return ad2s1210_get_lot_high_threshold(st, val, val2);
> + case IIO_EV_INFO_HYSTERESIS:
> + return ad2s1210_get_lot_low_threshold(st, val, val2);
> + default:
> + return -EINVAL;
> + }
> case IIO_PHASE:
> return ad2s1210_get_phase_lock_range(st, val, val2);
> default:
> @@ -759,6 +925,15 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
> struct ad2s1210_state *st = iio_priv(indio_dev);
>
> switch (chan->type) {
> + case IIO_ANGL:
> + switch (info) {
> + case IIO_EV_INFO_VALUE:
> + return ad2s1210_set_lot_high_threshold(st, val, val2);
> + case IIO_EV_INFO_HYSTERESIS:
> + return ad2s1210_set_lot_low_threshold(st, val, val2);
> + default:
> + return -EINVAL;
> + }
> case IIO_PHASE:
> return ad2s1210_set_phase_lock_range(st, val, val2);
> default:
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 08/17] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (6 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 07/17] staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:52 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 09/17] staging: iio: resolver: ad2s1210: convert DOS overrange " David Lechner
` (8 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
The AD2S1210 has a programmable threshold for the loss of signal (LOS)
fault. This fault is triggered when either the sine or cosine input
falls below the threshold voltage.
This patch converts the custom device LOS threshold attribute to an
event falling edge threshold attribute on a new monitor signal channel.
The monitor signal is an internal signal that combines the amplitudes
of the sine and cosine inputs as well as the current angle and position
output. This signal is used to detect faults in the input signals.
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 <dlechner@baylibre.com>
---
v4 changes:
* Fixed missing static qualifier on attribute definition.
v3 changes: This is a new patch in v3
drivers/staging/iio/resolver/ad2s1210.c | 76 +++++++++++++++++++++++++++++++--
1 file changed, 72 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 12437f697f79..d52aed30ca66 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -66,6 +66,11 @@
#define PHASE_360_DEG_TO_RAD_INT 6
#define PHASE_360_DEG_TO_RAD_MICRO 283185
+/* Threshold voltage registers have 1 LSB == 38 mV */
+#define THRESHOLD_MILLIVOLT_PER_LSB 38
+/* max voltage for threshold registers is 0x7F * 38 mV */
+#define THRESHOLD_RANGE_STR "[0 38 4826]"
+
enum ad2s1210_mode {
MOD_POS = 0b00,
MOD_VEL = 0b01,
@@ -451,6 +456,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
1237, /* 16-bit: same as 14-bit */
};
+static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st,
+ unsigned int reg, int *val)
+{
+ unsigned int reg_val;
+ int ret;
+
+ mutex_lock(&st->lock);
+ ret = regmap_read(st->regmap, reg, ®_val);
+ mutex_unlock(&st->lock);
+
+ if (ret < 0)
+ return ret;
+
+ *val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB;
+ return IIO_VAL_INT;
+}
+
+static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st,
+ unsigned int reg, int val)
+{
+ unsigned int reg_val;
+ int ret;
+
+ reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB;
+
+ mutex_lock(&st->lock);
+ ret = regmap_write(st->regmap, reg, reg_val);
+ mutex_unlock(&st->lock);
+
+ return ret;
+}
+
static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
int *val, int *val2)
{
@@ -710,9 +747,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(los_thrd, 0644,
- ad2s1210_show_reg, ad2s1210_store_reg,
- AD2S1210_REG_LOS_THRD);
static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644,
ad2s1210_show_reg, ad2s1210_store_reg,
AD2S1210_REG_DOS_OVR_THRD);
@@ -749,6 +783,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
},
};
+static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
+ {
+ /* Sine/cosine below LOS threshold fault. */
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ /* Loss of signal threshold. */
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
+};
+
static const struct iio_chan_spec ad2s1210_channels[] = {
{
.type = IIO_ANGL,
@@ -807,12 +851,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
.scan_index = -1,
.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
.info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
+ }, {
+ /* monitor signal */
+ .type = IIO_ALTVOLTAGE,
+ .indexed = 1,
+ .channel = 0,
+ .scan_index = -1,
+ .event_spec = ad2s1210_monitor_signal_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
},
};
static struct attribute *ad2s1210_attributes[] = {
&iio_dev_attr_fault.dev_attr.attr,
- &iio_dev_attr_los_thrd.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,
@@ -851,11 +902,14 @@ static IIO_CONST_ATTR(in_phase0_mag_rising_value_available,
__stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
__stringify(PHASE_360_DEG_TO_RAD_INT) "."
__stringify(PHASE_360_DEG_TO_RAD_MICRO));
+static IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available,
+ THRESHOLD_RANGE_STR);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
static struct attribute *ad2s1210_event_attributes[] = {
&iio_const_attr_in_phase0_mag_rising_value_available.dev_attr.attr,
+ &iio_const_attr_in_altvoltage0_thresh_falling_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,
@@ -908,6 +962,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_ALTVOLTAGE:
+ if (chan->output)
+ return -EINVAL;
+ if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
+ return ad2s1210_get_voltage_threshold(st,
+ AD2S1210_REG_LOS_THRD, val);
+ return -EINVAL;
case IIO_PHASE:
return ad2s1210_get_phase_lock_range(st, val, val2);
default:
@@ -934,6 +995,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
default:
return -EINVAL;
}
+ case IIO_ALTVOLTAGE:
+ if (chan->output)
+ return -EINVAL;
+ if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
+ return ad2s1210_set_voltage_threshold(st,
+ AD2S1210_REG_LOS_THRD, val);
+ return -EINVAL;
case IIO_PHASE:
return ad2s1210_set_phase_lock_range(st, val, val2);
default:
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 08/17] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
2023-10-06 0:50 ` [PATCH v4 08/17] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr David Lechner
@ 2023-10-10 15:52 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:52 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:25 -0500
David Lechner <dlechner@baylibre.com> wrote:
> The AD2S1210 has a programmable threshold for the loss of signal (LOS)
> fault. This fault is triggered when either the sine or cosine input
> falls below the threshold voltage.
>
> This patch converts the custom device LOS threshold attribute to an
> event falling edge threshold attribute on a new monitor signal channel.
> The monitor signal is an internal signal that combines the amplitudes
> of the sine and cosine inputs as well as the current angle and position
> output. This signal is used to detect faults in the input signals.
>
> The attribute now uses millivolts instead of the raw register value in
> accordance with the IIO ABI.
Hmm. The ABI is a bit ambigious on this front. When we have 'normal'
device attributes we tend to use the presence of _scale attributes
to scale both raw channels and the events associated with them.
I don't think we talk about what to do if that isn't available or
what happens for 'processed' channels - i.e. the ones already in
base units because those tend not to have associated events (as their
is normally nasty maths involved that is only sometimes reversible.)
We may want to clarify the ABI on this, but this is definitely
a valid option given there is no _scale for the channel.
Hence in interests of moving forwards I'm going to apply it.
Applied to the togreg branch of iio.git and pushed out as testing for
0-day to poke at it.
Thanks,
Jonathan
>
> Emitting the event will be implemented in a later patch.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>
> v4 changes:
> * Fixed missing static qualifier on attribute definition.
>
> v3 changes: This is a new patch in v3
>
> drivers/staging/iio/resolver/ad2s1210.c | 76 +++++++++++++++++++++++++++++++--
> 1 file changed, 72 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index 12437f697f79..d52aed30ca66 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -66,6 +66,11 @@
> #define PHASE_360_DEG_TO_RAD_INT 6
> #define PHASE_360_DEG_TO_RAD_MICRO 283185
>
> +/* Threshold voltage registers have 1 LSB == 38 mV */
> +#define THRESHOLD_MILLIVOLT_PER_LSB 38
> +/* max voltage for threshold registers is 0x7F * 38 mV */
> +#define THRESHOLD_RANGE_STR "[0 38 4826]"
> +
> enum ad2s1210_mode {
> MOD_POS = 0b00,
> MOD_VEL = 0b01,
> @@ -451,6 +456,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = {
> 1237, /* 16-bit: same as 14-bit */
> };
>
> +static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st,
> + unsigned int reg, int *val)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_read(st->regmap, reg, ®_val);
> + mutex_unlock(&st->lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + *val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB;
> + return IIO_VAL_INT;
> +}
> +
> +static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st,
> + unsigned int reg, int val)
> +{
> + unsigned int reg_val;
> + int ret;
> +
> + reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB;
> +
> + mutex_lock(&st->lock);
> + ret = regmap_write(st->regmap, reg, reg_val);
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
> int *val, int *val2)
> {
> @@ -710,9 +747,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(los_thrd, 0644,
> - ad2s1210_show_reg, ad2s1210_store_reg,
> - AD2S1210_REG_LOS_THRD);
> static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644,
> ad2s1210_show_reg, ad2s1210_store_reg,
> AD2S1210_REG_DOS_OVR_THRD);
> @@ -749,6 +783,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
> },
> };
>
> +static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
> + {
> + /* Sine/cosine below LOS threshold fault. */
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + /* Loss of signal threshold. */
> + .mask_separate = BIT(IIO_EV_INFO_VALUE),
> + },
> +};
> +
> static const struct iio_chan_spec ad2s1210_channels[] = {
> {
> .type = IIO_ANGL,
> @@ -807,12 +851,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
> .scan_index = -1,
> .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
> .info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY),
> + }, {
> + /* monitor signal */
> + .type = IIO_ALTVOLTAGE,
> + .indexed = 1,
> + .channel = 0,
> + .scan_index = -1,
> + .event_spec = ad2s1210_monitor_signal_event_spec,
> + .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec),
> },
> };
>
> static struct attribute *ad2s1210_attributes[] = {
> &iio_dev_attr_fault.dev_attr.attr,
> - &iio_dev_attr_los_thrd.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,
> @@ -851,11 +902,14 @@ static IIO_CONST_ATTR(in_phase0_mag_rising_value_available,
> __stringify(PHASE_44_DEG_TO_RAD_MICRO) " "
> __stringify(PHASE_360_DEG_TO_RAD_INT) "."
> __stringify(PHASE_360_DEG_TO_RAD_MICRO));
> +static IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available,
> + THRESHOLD_RANGE_STR);
> static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
> static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
>
> static struct attribute *ad2s1210_event_attributes[] = {
> &iio_const_attr_in_phase0_mag_rising_value_available.dev_attr.attr,
> + &iio_const_attr_in_altvoltage0_thresh_falling_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,
> @@ -908,6 +962,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_ALTVOLTAGE:
> + if (chan->output)
> + return -EINVAL;
> + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
> + return ad2s1210_get_voltage_threshold(st,
> + AD2S1210_REG_LOS_THRD, val);
> + return -EINVAL;
> case IIO_PHASE:
> return ad2s1210_get_phase_lock_range(st, val, val2);
> default:
> @@ -934,6 +995,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
> default:
> return -EINVAL;
> }
> + case IIO_ALTVOLTAGE:
> + if (chan->output)
> + return -EINVAL;
> + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING)
> + return ad2s1210_set_voltage_threshold(st,
> + AD2S1210_REG_LOS_THRD, val);
> + return -EINVAL;
> case IIO_PHASE:
> return ad2s1210_set_phase_lock_range(st, val, val2);
> default:
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 09/17] staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (7 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 08/17] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:55 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 10/17] staging: iio: resolver: ad2s1210: convert DOS mismatch " David Lechner
` (7 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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 above 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 <dlechner@baylibre.com>
---
v4 changes:
* Fixed missing word in commit message.
* Fixed missing static qualifier on attribute definition.
v3 changes: This is a new patch in v3
drivers/staging/iio/resolver/ad2s1210.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index d52aed30ca66..3c224bbeae17 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -747,9 +747,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);
@@ -791,6 +788,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[] = {
@@ -864,7 +868,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,
@@ -904,12 +907,15 @@ static IIO_CONST_ATTR(in_phase0_mag_rising_value_available,
__stringify(PHASE_360_DEG_TO_RAD_MICRO));
static IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available,
THRESHOLD_RANGE_STR);
+static IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available,
+ THRESHOLD_RANGE_STR);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
static struct attribute *ad2s1210_event_attributes[] = {
&iio_const_attr_in_phase0_mag_rising_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,
@@ -968,6 +974,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);
@@ -1001,6 +1010,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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 09/17] staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr
2023-10-06 0:50 ` [PATCH v4 09/17] staging: iio: resolver: ad2s1210: convert DOS overrange " David Lechner
@ 2023-10-10 15:55 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:55 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:26 -0500
David Lechner <dlechner@baylibre.com> wrote:
> 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 above 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 <dlechner@baylibre.com>
Applied
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 10/17] staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (8 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 09/17] staging: iio: resolver: ad2s1210: convert DOS overrange " David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:56 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 11/17] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs David Lechner
` (6 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
The AD2S1210 has a programmable threshold for the degradation of signal
(DOS) mismatch fault. This fault is triggered when the difference in
voltage between the sine and cosine inputs exceeds the threshold. In
other words, when the magnitude of sine and cosine inputs are equal,
the AC component of the monitor signal is zero and when the magnitudes
of the sine and cosine inputs are not equal, the AC component of the
monitor signal is the difference between the sine and cosine inputs.
So the fault occurs when the magnitude of the AC component of the
monitor signal exceeds the DOS mismatch threshold voltage.
This patch converts the custom device DOS mismatch threshold attribute
to an event magnitude 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 <dlechner@baylibre.com>
---
v4 changes:
* Changed event direction from none to rising.
* Fixed missing static qualifier on attribute definition.
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 3c224bbeae17..870c4a9a6214 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -747,9 +747,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_mis_thrd, 0644,
- ad2s1210_show_reg, ad2s1210_store_reg,
- AD2S1210_REG_DOS_MIS_THRD);
static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644,
ad2s1210_show_reg, ad2s1210_store_reg,
AD2S1210_REG_DOS_RST_MAX_THRD);
@@ -795,6 +792,12 @@ static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = {
/* Degredation of signal overrange threshold. */
.mask_separate = BIT(IIO_EV_INFO_VALUE),
},
+ {
+ /* Sine/cosine DOS mismatch fault.*/
+ .type = IIO_EV_TYPE_MAG,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE),
+ },
};
static const struct iio_chan_spec ad2s1210_channels[] = {
@@ -868,7 +871,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_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,
NULL,
@@ -909,6 +911,8 @@ static IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available,
THRESHOLD_RANGE_STR);
static IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available,
THRESHOLD_RANGE_STR);
+static IIO_CONST_ATTR(in_altvoltage0_mag_rising_value_available,
+ THRESHOLD_RANGE_STR);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
@@ -916,6 +920,7 @@ static struct attribute *ad2s1210_event_attributes[] = {
&iio_const_attr_in_phase0_mag_rising_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_const_attr_in_altvoltage0_mag_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,
@@ -977,6 +982,9 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev,
if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_RISING)
return ad2s1210_get_voltage_threshold(st,
AD2S1210_REG_DOS_OVR_THRD, val);
+ if (type == IIO_EV_TYPE_MAG)
+ return ad2s1210_get_voltage_threshold(st,
+ AD2S1210_REG_DOS_MIS_THRD, val);
return -EINVAL;
case IIO_PHASE:
return ad2s1210_get_phase_lock_range(st, val, val2);
@@ -1013,6 +1021,9 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_RISING)
return ad2s1210_set_voltage_threshold(st,
AD2S1210_REG_DOS_OVR_THRD, val);
+ if (type == IIO_EV_TYPE_MAG)
+ return ad2s1210_set_voltage_threshold(st,
+ AD2S1210_REG_DOS_MIS_THRD, val);
return -EINVAL;
case IIO_PHASE:
return ad2s1210_set_phase_lock_range(st, val, val2);
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 10/17] staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr
2023-10-06 0:50 ` [PATCH v4 10/17] staging: iio: resolver: ad2s1210: convert DOS mismatch " David Lechner
@ 2023-10-10 15:56 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:56 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:27 -0500
David Lechner <dlechner@baylibre.com> wrote:
> The AD2S1210 has a programmable threshold for the degradation of signal
> (DOS) mismatch fault. This fault is triggered when the difference in
> voltage between the sine and cosine inputs exceeds the threshold. In
> other words, when the magnitude of sine and cosine inputs are equal,
> the AC component of the monitor signal is zero and when the magnitudes
> of the sine and cosine inputs are not equal, the AC component of the
> monitor signal is the difference between the sine and cosine inputs.
> So the fault occurs when the magnitude of the AC component of the
> monitor signal exceeds the DOS mismatch threshold voltage.
>
> This patch converts the custom device DOS mismatch threshold attribute
> to an event magnitude 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 <dlechner@baylibre.com>
Applied,
Thanks,
Jonathan
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 11/17] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (9 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 10/17] staging: iio: resolver: ad2s1210: convert DOS mismatch " David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:57 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 12/17] iio: event: add optional event label support David Lechner
` (5 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes:
* Fixed name of attributes in sysfs docs.
* Changed event direction from none to rising.
* Fixed missing static qualifier on attribute definition.
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..f92c79342b93
--- /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_mag_rising_reset_max
+KernelVersion: 6.7
+Contact: linux-iio@vger.kernel.org
+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_mag_rising_reset_max_available
+KernelVersion: 6.7
+Contact: linux-iio@vger.kernel.org
+Description:
+ Reading returns the allowable voltage range for
+ in_altvoltage0_mag_rising_reset_max.
+
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0_mag_rising_reset_min
+KernelVersion: 6.7
+Contact: linux-iio@vger.kernel.org
+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_mag_rising_reset_min_available
+KernelVersion: 6.7
+Contact: linux-iio@vger.kernel.org
+Description:
+ Reading returns the allowable voltage range for
+ in_altvoltage0_mag_rising_reset_min.
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 870c4a9a6214..9fac806c2a5f 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -286,41 +286,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)
@@ -747,13 +712,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. */
@@ -871,8 +829,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,
};
@@ -880,6 +836,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,
@@ -913,6 +912,14 @@ static IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available,
THRESHOLD_RANGE_STR);
static IIO_CONST_ATTR(in_altvoltage0_mag_rising_value_available,
THRESHOLD_RANGE_STR);
+static IIO_DEVICE_ATTR(in_altvoltage0_mag_rising_reset_max, 0644,
+ event_attr_voltage_reg_show, event_attr_voltage_reg_store,
+ AD2S1210_REG_DOS_RST_MAX_THRD);
+static IIO_CONST_ATTR(in_altvoltage0_mag_rising_reset_max_available, THRESHOLD_RANGE_STR);
+static IIO_DEVICE_ATTR(in_altvoltage0_mag_rising_reset_min, 0644,
+ event_attr_voltage_reg_show, event_attr_voltage_reg_store,
+ AD2S1210_REG_DOS_RST_MIN_THRD);
+static IIO_CONST_ATTR(in_altvoltage0_mag_rising_reset_min_available, THRESHOLD_RANGE_STR);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0);
static IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0);
@@ -921,6 +928,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_rising_value_available.dev_attr.attr,
+ &iio_dev_attr_in_altvoltage0_mag_rising_reset_max.dev_attr.attr,
+ &iio_const_attr_in_altvoltage0_mag_rising_reset_max_available.dev_attr.attr,
+ &iio_dev_attr_in_altvoltage0_mag_rising_reset_min.dev_attr.attr,
+ &iio_const_attr_in_altvoltage0_mag_rising_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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 11/17] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs
2023-10-06 0:50 ` [PATCH v4 11/17] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs David Lechner
@ 2023-10-10 15:57 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:57 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:28 -0500
David Lechner <dlechner@baylibre.com> wrote:
> 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.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 12/17] iio: event: add optional event label support
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (10 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 11/17] staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 15:59 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 13/17] staging: iio: resolver: ad2s1210: implement fault events David Lechner
` (4 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
This adds a new optional field to struct iio_info to allow drivers to
specify a label for the event. This is useful for cases where there are
many events or the event attribute name is not descriptive enough or
where an event doesn't have any other attributes.
The implementation is based on the existing label support for channels.
So either all events of a device have a label attribute or none do.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: New patch in v4.
drivers/iio/industrialio-event.c | 55 ++++++++++++++++++++++++++++++++++++++++
include/linux/iio/iio.h | 8 ++++++
2 files changed, 63 insertions(+)
diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
index 19f7a91157ee..910c1f14abd5 100644
--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -355,6 +355,21 @@ static ssize_t iio_ev_value_store(struct device *dev,
return len;
}
+static ssize_t iio_ev_label_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+ if (indio_dev->info->read_event_label)
+ return indio_dev->info->read_event_label(indio_dev,
+ this_attr->c, iio_ev_attr_type(this_attr),
+ iio_ev_attr_dir(this_attr), buf);
+
+ return -EINVAL;
+}
+
static int iio_device_add_event(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, unsigned int spec_index,
enum iio_event_type type, enum iio_event_direction dir,
@@ -411,6 +426,41 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
return attrcount;
}
+static int iio_device_add_event_label(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int spec_index,
+ enum iio_event_type type,
+ enum iio_event_direction dir)
+{
+ struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
+ char *postfix;
+ int ret;
+
+ if (!indio_dev->info->read_event_label)
+ return 0;
+
+ if (dir != IIO_EV_DIR_NONE)
+ postfix = kasprintf(GFP_KERNEL, "%s_%s_label",
+ iio_ev_type_text[type],
+ iio_ev_dir_text[dir]);
+ else
+ postfix = kasprintf(GFP_KERNEL, "%s_label",
+ iio_ev_type_text[type]);
+ if (postfix == NULL)
+ return -ENOMEM;
+
+ ret = __iio_add_chan_devattr(postfix, chan, &iio_ev_label_show, NULL,
+ spec_index, IIO_SEPARATE, &indio_dev->dev, NULL,
+ &iio_dev_opaque->event_interface->dev_attr_list);
+
+ kfree(postfix);
+
+ if (ret < 0)
+ return ret;
+
+ return 1;
+}
+
static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan)
{
@@ -448,6 +498,11 @@ static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
if (ret < 0)
return ret;
attrcount += ret;
+
+ ret = iio_device_add_event_label(indio_dev, chan, i, type, dir);
+ if (ret < 0)
+ return ret;
+ attrcount += ret;
}
ret = attrcount;
return ret;
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 7bfa1b9bc8a2..d0ce3b71106a 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -427,6 +427,8 @@ struct iio_trigger; /* forward declaration */
* @write_event_config: set if the event is enabled.
* @read_event_value: read a configuration value associated with the event.
* @write_event_value: write a configuration value for the event.
+ * @read_event_label: function to request label name for a specified label,
+ * for better event identification.
* @validate_trigger: function to validate the trigger when the
* current trigger gets changed.
* @update_scan_mode: function to configure device and scan buffer when
@@ -511,6 +513,12 @@ struct iio_info {
enum iio_event_direction dir,
enum iio_event_info info, int val, int val2);
+ int (*read_event_label)(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ char *label);
+
int (*validate_trigger)(struct iio_dev *indio_dev,
struct iio_trigger *trig);
int (*update_scan_mode)(struct iio_dev *indio_dev,
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 12/17] iio: event: add optional event label support
2023-10-06 0:50 ` [PATCH v4 12/17] iio: event: add optional event label support David Lechner
@ 2023-10-10 15:59 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 15:59 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:29 -0500
David Lechner <dlechner@baylibre.com> wrote:
> This adds a new optional field to struct iio_info to allow drivers to
> specify a label for the event. This is useful for cases where there are
> many events or the event attribute name is not descriptive enough or
> where an event doesn't have any other attributes.
>
> The implementation is based on the existing label support for channels.
> So either all events of a device have a label attribute or none do.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Was going to moan about lack of docs, but I see they are in later patches.
Ok. General feature is fine. Applied.
Jonathan
> ---
>
> v4 changes: New patch in v4.
>
> drivers/iio/industrialio-event.c | 55 ++++++++++++++++++++++++++++++++++++++++
> include/linux/iio/iio.h | 8 ++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/drivers/iio/industrialio-event.c b/drivers/iio/industrialio-event.c
> index 19f7a91157ee..910c1f14abd5 100644
> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -355,6 +355,21 @@ static ssize_t iio_ev_value_store(struct device *dev,
> return len;
> }
>
> +static ssize_t iio_ev_label_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> + if (indio_dev->info->read_event_label)
> + return indio_dev->info->read_event_label(indio_dev,
> + this_attr->c, iio_ev_attr_type(this_attr),
> + iio_ev_attr_dir(this_attr), buf);
> +
> + return -EINVAL;
> +}
> +
> static int iio_device_add_event(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan, unsigned int spec_index,
> enum iio_event_type type, enum iio_event_direction dir,
> @@ -411,6 +426,41 @@ static int iio_device_add_event(struct iio_dev *indio_dev,
> return attrcount;
> }
>
> +static int iio_device_add_event_label(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + unsigned int spec_index,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev);
> + char *postfix;
> + int ret;
> +
> + if (!indio_dev->info->read_event_label)
> + return 0;
> +
> + if (dir != IIO_EV_DIR_NONE)
> + postfix = kasprintf(GFP_KERNEL, "%s_%s_label",
> + iio_ev_type_text[type],
> + iio_ev_dir_text[dir]);
> + else
> + postfix = kasprintf(GFP_KERNEL, "%s_label",
> + iio_ev_type_text[type]);
> + if (postfix == NULL)
> + return -ENOMEM;
> +
> + ret = __iio_add_chan_devattr(postfix, chan, &iio_ev_label_show, NULL,
> + spec_index, IIO_SEPARATE, &indio_dev->dev, NULL,
> + &iio_dev_opaque->event_interface->dev_attr_list);
> +
> + kfree(postfix);
> +
> + if (ret < 0)
> + return ret;
> +
> + return 1;
> +}
> +
> static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan)
> {
> @@ -448,6 +498,11 @@ static int iio_device_add_event_sysfs(struct iio_dev *indio_dev,
> if (ret < 0)
> return ret;
> attrcount += ret;
> +
> + ret = iio_device_add_event_label(indio_dev, chan, i, type, dir);
> + if (ret < 0)
> + return ret;
> + attrcount += ret;
> }
> ret = attrcount;
> return ret;
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 7bfa1b9bc8a2..d0ce3b71106a 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -427,6 +427,8 @@ struct iio_trigger; /* forward declaration */
> * @write_event_config: set if the event is enabled.
> * @read_event_value: read a configuration value associated with the event.
> * @write_event_value: write a configuration value for the event.
> + * @read_event_label: function to request label name for a specified label,
> + * for better event identification.
> * @validate_trigger: function to validate the trigger when the
> * current trigger gets changed.
> * @update_scan_mode: function to configure device and scan buffer when
> @@ -511,6 +513,12 @@ struct iio_info {
> enum iio_event_direction dir,
> enum iio_event_info info, int val, int val2);
>
> + int (*read_event_label)(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + char *label);
> +
> int (*validate_trigger)(struct iio_dev *indio_dev,
> struct iio_trigger *trig);
> int (*update_scan_mode)(struct iio_dev *indio_dev,
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 13/17] staging: iio: resolver: ad2s1210: implement fault events
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (11 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 12/17] iio: event: add optional event label support David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 16:05 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 14/17] staging: iio: resolver: ad2s1210: add register/fault support summary David Lechner
` (3 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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 type in order to
have a unique event for each fault.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes:
* Added sysfs docs for *_label attributes.
* Added implementation of read_event_label.
* Dropped use of IIO_MOD_X_OR_Y.
* Tweaked event type/direction as in previous patches.
* Fixed build error due to st->rx[2].
v3 changes: This is a new patch in v3
Documentation/ABI/testing/sysfs-bus-iio | 15 +++
drivers/staging/iio/resolver/ad2s1210.c | 211 +++++++++++++++++++++++++++++---
2 files changed, 212 insertions(+), 14 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 8e13642bbe23..19cde14f3869 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -2239,3 +2239,18 @@ Contact: linux-iio@vger.kernel.org
Description:
The x and y light color coordinate on the CIE 1931 chromaticity
diagram.
+
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltageY_mag_either_label
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltageY_mag_rising_label
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltageY_thresh_falling_label
+What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltageY_thresh_rising_label
+What: /sys/bus/iio/devices/iio:deviceX/events/in_anglvelY_mag_rising_label
+What: /sys/bus/iio/devices/iio:deviceX/events/in_anglY_thresh_rising_label
+What: /sys/bus/iio/devices/iio:deviceX/events/in_phaseY_mag_rising_label
+KernelVersion: 6.7
+Contact: linux-iio@vger.kernel.org
+Description:
+ Optional symbolic label to a device channel event.
+ If a label is defined for this event add that to the event
+ specific attributes. This is useful for userspace to be able to
+ better identify an individual event.
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 9fac806c2a5f..d9d51bbbade8 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,
@@ -100,8 +113,13 @@ struct ad2s1210_state {
int hysteresis_available[2];
/** 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];
@@ -160,7 +178,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;
}
/*
@@ -203,6 +229,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
@@ -286,14 +316,101 @@ 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)) {
+ /*
+ * The chip does not differentiate between fault on sine vs.
+ * cosine channel so we just send an event on both channels.
+ */
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 1,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_EITHER),
+ timestamp);
+ iio_push_event(indio_dev,
+ IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 2,
+ IIO_EV_TYPE_MAG,
+ IIO_EV_DIR_EITHER),
+ 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_RISING),
+ 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_RISING),
+ 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);
@@ -310,17 +427,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:
@@ -328,6 +445,8 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st,
break;
}
+ ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
+
error_ret:
gpiod_set_value(st->sample_gpio, 0);
/* delay (2 * tck + 20) nano seconds */
@@ -611,7 +730,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:
@@ -725,6 +844,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_MAG,
+ .dir = IIO_EV_DIR_RISING,
+ },
+};
+
static const struct iio_event_spec ad2s1210_phase_event_spec[] = {
{
/* Phase error fault. */
@@ -758,6 +885,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_EITHER,
+ },
+};
+
static const struct iio_chan_spec ad2s1210_channels[] = {
{
.type = IIO_ANGL,
@@ -788,6 +923,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),
{
@@ -824,6 +961,22 @@ 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,
+ .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 = 2,
+ .scan_index = -1,
+ .event_spec = ad2s1210_sin_cos_event_spec,
+ .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec),
},
};
@@ -943,7 +1096,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);
@@ -1043,6 +1196,36 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev,
}
}
+static int ad2s1210_read_event_label(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ enum iio_event_type type,
+ enum iio_event_direction dir,
+ char *label)
+{
+ if (chan->type == IIO_ANGL)
+ return sprintf(label, "LOT\n");
+ if (chan->type == IIO_ANGL_VEL)
+ return sprintf(label, "max tracking rate\n");
+ if (chan->type == IIO_PHASE)
+ return sprintf(label, "phase lock\n");
+ if (chan->type == IIO_ALTVOLTAGE) {
+ if (chan->channel == 0) {
+ if (type == IIO_EV_TYPE_THRESH &&
+ dir == IIO_EV_DIR_FALLING)
+ return sprintf(label, "LOS\n");
+ if (type == IIO_EV_TYPE_THRESH &&
+ dir == IIO_EV_DIR_RISING)
+ return sprintf(label, "DOS overrange\n");
+ if (type == IIO_EV_TYPE_MAG)
+ return sprintf(label, "DOS mismatch\n");
+ }
+ if (chan->channel == 1 || chan->channel == 2)
+ return sprintf(label, "clipped\n");
+ }
+
+ return -EINVAL;
+}
+
static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int reg, unsigned int writeval,
unsigned int *readval)
@@ -1080,12 +1263,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)) {
@@ -1093,14 +1275,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:
@@ -1119,6 +1301,7 @@ static const struct iio_info ad2s1210_info = {
.attrs = &ad2s1210_attribute_group,
.read_event_value = ad2s1210_read_event_value,
.write_event_value = ad2s1210_write_event_value,
+ .read_event_label = ad2s1210_read_event_label,
.debugfs_reg_access = &ad2s1210_debugfs_reg_access,
};
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 13/17] staging: iio: resolver: ad2s1210: implement fault events
2023-10-06 0:50 ` [PATCH v4 13/17] staging: iio: resolver: ad2s1210: implement fault events David Lechner
@ 2023-10-10 16:05 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 16:05 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:30 -0500
David Lechner <dlechner@baylibre.com> wrote:
> 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 type in order to
> have a unique event for each fault.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 14/17] staging: iio: resolver: ad2s1210: add register/fault support summary
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (12 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 13/17] staging: iio: resolver: ad2s1210: implement fault events David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 16:07 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 15/17] staging: iio: resolver: ad2s1210: add label attribute support David Lechner
` (2 subsequent siblings)
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
The ad2s1210 driver shoe-horns the register and fault support into IIO
events. The mapping between the registers/faults and the events is not
obvious. To save users from having to read the entire driver to figure
out how to use it, add a summary of the register/fault support to the
top of the file.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: New patch in v4.
drivers/staging/iio/resolver/ad2s1210.c | 40 +++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index d9d51bbbade8..51490fea1647 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -4,7 +4,47 @@
*
* Copyright (c) 2010-2010 Analog Devices Inc.
* Copyright (c) 2023 BayLibre, SAS
+ *
+ * Device register to IIO ABI mapping:
+ *
+ * Register | Addr | IIO ABI (sysfs)
+ * ----------------------------|------|-------------------------------------------
+ * DOS Overrange Threshold | 0x89 | events/in_altvoltage0_thresh_rising_value
+ * DOS Mismatch Threshold | 0x8A | events/in_altvoltage0_mag_rising_value
+ * DOS Reset Maximum Threshold | 0x8B | events/in_altvoltage0_mag_rising_reset_max
+ * DOS Reset Minimum Threshold | 0x8C | events/in_altvoltage0_mag_rising_reset_min
+ * LOT High Threshold | 0x8D | events/in_angl1_thresh_rising_value
+ * LOT Low Threshold [1] | 0x8E | events/in_angl1_thresh_rising_hysteresis
+ * Excitation Frequency | 0x91 | out_altvoltage0_frequency
+ * Control | 0x92 | *as bit fields*
+ * Phase lock range | D5 | events/in_phase0_mag_rising_value
+ * Hysteresis | D4 | in_angl0_hysteresis
+ * Encoder resolution | D3:2 | *not implemented*
+ * Resolution | D1:0 | *device tree: assigned-resolution-bits*
+ * Soft Reset | 0xF0 | [2]
+ * Fault | 0xFF | *not implemented*
+ *
+ * [1]: The value written to the LOT low register is high value minus the
+ * hysteresis.
+ * [2]: Soft reset is performed when `out_altvoltage0_frequency` is written.
+ *
+ * Fault to event mapping:
+ *
+ * Fault | | Channel | Type | Direction
+ * ----------------------------------------|----|---------------------------------
+ * Sine/cosine inputs clipped [3] | D7 | altvoltage1 | mag | either
+ * Sine/cosine inputs below LOS | D6 | altvoltage0 | thresh | falling
+ * Sine/cosine inputs exceed DOS overrange | D5 | altvoltage0 | thresh | rising
+ * Sine/cosine inputs exceed DOS mismatch | D4 | altvoltage0 | mag | rising
+ * Tracking error exceeds LOT | D3 | angl1 | thresh | rising
+ * Velocity exceeds maximum tracking rate | D2 | anglvel0 | mag | rising
+ * Phase error exceeds phase lock range | D1 | phase0 | mag | rising
+ * Configuration parity error | D0 | *writes to kernel log*
+ *
+ * [3]: The chip does not differentiate between fault on sine vs. cosine so
+ * there will also be an event on the altvoltage2 channel.
*/
+
#include <linux/bitfield.h>
#include <linux/bits.h>
#include <linux/clk.h>
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 14/17] staging: iio: resolver: ad2s1210: add register/fault support summary
2023-10-06 0:50 ` [PATCH v4 14/17] staging: iio: resolver: ad2s1210: add register/fault support summary David Lechner
@ 2023-10-10 16:07 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 16:07 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:31 -0500
David Lechner <dlechner@baylibre.com> wrote:
> The ad2s1210 driver shoe-horns the register and fault support into IIO
> events. The mapping between the registers/faults and the events is not
> obvious. To save users from having to read the entire driver to figure
> out how to use it, add a summary of the register/fault support to the
> top of the file.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
This is indeed useful to have.
Applied.
Thanks,
Jonathan
> ---
>
> v4 changes: New patch in v4.
>
> drivers/staging/iio/resolver/ad2s1210.c | 40 +++++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index d9d51bbbade8..51490fea1647 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -4,7 +4,47 @@
> *
> * Copyright (c) 2010-2010 Analog Devices Inc.
> * Copyright (c) 2023 BayLibre, SAS
> + *
> + * Device register to IIO ABI mapping:
> + *
> + * Register | Addr | IIO ABI (sysfs)
> + * ----------------------------|------|-------------------------------------------
> + * DOS Overrange Threshold | 0x89 | events/in_altvoltage0_thresh_rising_value
> + * DOS Mismatch Threshold | 0x8A | events/in_altvoltage0_mag_rising_value
> + * DOS Reset Maximum Threshold | 0x8B | events/in_altvoltage0_mag_rising_reset_max
> + * DOS Reset Minimum Threshold | 0x8C | events/in_altvoltage0_mag_rising_reset_min
> + * LOT High Threshold | 0x8D | events/in_angl1_thresh_rising_value
> + * LOT Low Threshold [1] | 0x8E | events/in_angl1_thresh_rising_hysteresis
> + * Excitation Frequency | 0x91 | out_altvoltage0_frequency
> + * Control | 0x92 | *as bit fields*
> + * Phase lock range | D5 | events/in_phase0_mag_rising_value
> + * Hysteresis | D4 | in_angl0_hysteresis
> + * Encoder resolution | D3:2 | *not implemented*
> + * Resolution | D1:0 | *device tree: assigned-resolution-bits*
> + * Soft Reset | 0xF0 | [2]
> + * Fault | 0xFF | *not implemented*
> + *
> + * [1]: The value written to the LOT low register is high value minus the
> + * hysteresis.
> + * [2]: Soft reset is performed when `out_altvoltage0_frequency` is written.
> + *
> + * Fault to event mapping:
> + *
> + * Fault | | Channel | Type | Direction
> + * ----------------------------------------|----|---------------------------------
> + * Sine/cosine inputs clipped [3] | D7 | altvoltage1 | mag | either
> + * Sine/cosine inputs below LOS | D6 | altvoltage0 | thresh | falling
> + * Sine/cosine inputs exceed DOS overrange | D5 | altvoltage0 | thresh | rising
> + * Sine/cosine inputs exceed DOS mismatch | D4 | altvoltage0 | mag | rising
> + * Tracking error exceeds LOT | D3 | angl1 | thresh | rising
> + * Velocity exceeds maximum tracking rate | D2 | anglvel0 | mag | rising
> + * Phase error exceeds phase lock range | D1 | phase0 | mag | rising
> + * Configuration parity error | D0 | *writes to kernel log*
> + *
> + * [3]: The chip does not differentiate between fault on sine vs. cosine so
> + * there will also be an event on the altvoltage2 channel.
> */
> +
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> #include <linux/clk.h>
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 15/17] staging: iio: resolver: ad2s1210: add label attribute support
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (13 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 14/17] staging: iio: resolver: ad2s1210: add register/fault support summary David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 16:08 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 16/17] staging: iio: resolver: ad2s1210: remove fault attribute David Lechner
2023-10-06 0:50 ` [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex) David Lechner
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
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 <dlechner@baylibre.com>
---
v4 changes:
* Adjusted for channel rearrangement in previous patches.
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 51490fea1647..59c8eed26701 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -1158,6 +1158,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)
+ return sprintf(label, "cosine\n");
+ if (chan->channel == 2)
+ 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,
@@ -1338,6 +1366,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
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v4 16/17] staging: iio: resolver: ad2s1210: remove fault attribute
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (14 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 15/17] staging: iio: resolver: ad2s1210: add label attribute support David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 16:09 ` Jonathan Cameron
2023-10-06 0:50 ` [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex) David Lechner
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
Faults have been converted to events and we are now polling the fault
register each time we read a sample, so we no longer need the fault
attribute.
This attribute was not suitable for promotion out of staging anyway
since it was returning multiple values in a single attribute.
The fault clearing feature should not be needed unless we need to
support the fault output pins on the chip which is not currently
supported. So we can add this feature back in if we need it later.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: New patch in v4.
drivers/staging/iio/resolver/ad2s1210.c | 57 ---------------------------------
1 file changed, 57 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index 59c8eed26701..c4e1bc22e8b0 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -312,50 +312,6 @@ static int ad2s1210_reinit_excitation_frequency(struct ad2s1210_state *st,
return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0);
}
-/* read the fault register since last sample */
-static ssize_t ad2s1210_show_fault(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_FAULT, &value);
- mutex_unlock(&st->lock);
-
- return ret < 0 ? ret : sprintf(buf, "0x%02x\n", value);
-}
-
-static ssize_t ad2s1210_clear_fault(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 int value;
- int ret;
-
- mutex_lock(&st->lock);
-
- gpiod_set_value(st->sample_gpio, 1);
- /* delay (2 * tck + 20) nano seconds */
- udelay(1);
- gpiod_set_value(st->sample_gpio, 0);
-
- ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &value);
- if (ret < 0)
- goto error_ret;
-
- gpiod_set_value(st->sample_gpio, 1);
- gpiod_set_value(st->sample_gpio, 0);
-
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret < 0 ? ret : len;
-}
-
static void ad2s1210_push_events(struct iio_dev *indio_dev,
u8 flags, s64 timestamp)
{
@@ -868,9 +824,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 const struct iio_event_spec ad2s1210_position_event_spec[] = {
{
/* Tracking error exceeds LOT threshold fault. */
@@ -1020,15 +973,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = {
},
};
-static struct attribute *ad2s1210_attributes[] = {
- &iio_dev_attr_fault.dev_attr.attr,
- NULL,
-};
-
-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)
@@ -1367,7 +1311,6 @@ static const struct iio_info ad2s1210_info = {
.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,
.read_event_label = ad2s1210_read_event_label,
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 16/17] staging: iio: resolver: ad2s1210: remove fault attribute
2023-10-06 0:50 ` [PATCH v4 16/17] staging: iio: resolver: ad2s1210: remove fault attribute David Lechner
@ 2023-10-10 16:09 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 16:09 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:33 -0500
David Lechner <dlechner@baylibre.com> wrote:
> Faults have been converted to events and we are now polling the fault
> register each time we read a sample, so we no longer need the fault
> attribute.
>
> This attribute was not suitable for promotion out of staging anyway
> since it was returning multiple values in a single attribute.
>
> The fault clearing feature should not be needed unless we need to
> support the fault output pins on the chip which is not currently
> supported. So we can add this feature back in if we need it later.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Applied
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex)
2023-10-06 0:50 [PATCH v4 00/17] iio: resolver: move ad2s1210 out of staging David Lechner
` (15 preceding siblings ...)
2023-10-06 0:50 ` [PATCH v4 16/17] staging: iio: resolver: ad2s1210: remove fault attribute David Lechner
@ 2023-10-06 0:50 ` David Lechner
2023-10-10 16:17 ` Jonathan Cameron
16 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-06 0:50 UTC (permalink / raw)
To: linux-iio, linux-staging
Cc: David Lechner, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
We can simplify the code and get rid of most of the gotos by using
guard(mutex) from cleanup.h.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v4 changes: New patch in v4.
drivers/staging/iio/resolver/ad2s1210.c | 157 ++++++++++----------------------
1 file changed, 50 insertions(+), 107 deletions(-)
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
index c4e1bc22e8b0..c4e0ffa92dc2 100644
--- a/drivers/staging/iio/resolver/ad2s1210.c
+++ b/drivers/staging/iio/resolver/ad2s1210.c
@@ -47,6 +47,7 @@
#include <linux/bitfield.h>
#include <linux/bits.h>
+#include <linux/cleanup.h>
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/device.h>
@@ -404,11 +405,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
s64 timestamp;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
+
gpiod_set_value(st->sample_gpio, 1);
timestamp = iio_get_time_ns(indio_dev);
/* delay (6 * tck + 20) nano seconds */
udelay(1);
+ gpiod_set_value(st->sample_gpio, 0);
switch (chan->type) {
case IIO_ANGL:
@@ -418,14 +421,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
ret = ad2s1210_set_mode(st, MOD_VEL);
break;
default:
- ret = -EINVAL;
- break;
+ return -EINVAL;
}
if (ret < 0)
- goto error_ret;
+ return ret;
ret = spi_read(st->sdev, &st->sample, 3);
if (ret < 0)
- goto error_ret;
+ return ret;
switch (chan->type) {
case IIO_ANGL:
@@ -437,17 +439,11 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
ret = IIO_VAL_INT;
break;
default:
- ret = -EINVAL;
- break;
+ return -EINVAL;
}
ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
-error_ret:
- gpiod_set_value(st->sample_gpio, 0);
- /* delay (2 * tck + 20) nano seconds */
- udelay(1);
- mutex_unlock(&st->lock);
return ret;
}
@@ -455,11 +451,9 @@ static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val)
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = regmap_test_bits(st->regmap, AD2S1210_REG_CONTROL,
AD2S1210_ENABLE_HYSTERESIS);
- mutex_unlock(&st->lock);
-
if (ret < 0)
return ret;
@@ -469,15 +463,10 @@ static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val)
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;
+ guard(mutex)(&st->lock);
+ return regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
+ AD2S1210_ENABLE_HYSTERESIS,
+ val ? AD2S1210_ENABLE_HYSTERESIS : 0);
}
static int ad2s1210_get_phase_lock_range(struct ad2s1210_state *st,
@@ -485,11 +474,9 @@ static int ad2s1210_get_phase_lock_range(struct ad2s1210_state *st,
{
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&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;
@@ -509,7 +496,7 @@ static int ad2s1210_get_phase_lock_range(struct ad2s1210_state *st,
static int ad2s1210_set_phase_lock_range(struct ad2s1210_state *st,
int val, int val2)
{
- int deg, ret;
+ int deg;
/* convert radians to degrees - only two allowable values */
if (val == PHASE_44_DEG_TO_RAD_INT && val2 == PHASE_44_DEG_TO_RAD_MICRO)
@@ -520,12 +507,10 @@ static int ad2s1210_set_phase_lock_range(struct ad2s1210_state *st,
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;
+ guard(mutex)(&st->lock);
+ return regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL,
+ AD2S1210_PHASE_LOCK_RANGE_44,
+ deg == 44 ? AD2S1210_PHASE_LOCK_RANGE_44 : 0);
}
/* map resolution to microradians/LSB for LOT registers */
@@ -542,10 +527,8 @@ static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st,
unsigned int reg_val;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = regmap_read(st->regmap, reg, ®_val);
- mutex_unlock(&st->lock);
-
if (ret < 0)
return ret;
@@ -557,15 +540,11 @@ static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st,
unsigned int reg, int val)
{
unsigned int reg_val;
- int ret;
reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB;
- mutex_lock(&st->lock);
- ret = regmap_write(st->regmap, reg, reg_val);
- mutex_unlock(&st->lock);
-
- return ret;
+ guard(mutex)(&st->lock);
+ return regmap_write(st->regmap, reg, reg_val);
}
static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
@@ -574,10 +553,8 @@ static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st,
unsigned int reg_val;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, ®_val);
- mutex_unlock(&st->lock);
-
if (ret < 0)
return ret;
@@ -596,18 +573,18 @@ static int ad2s1210_set_lot_high_threshold(struct ad2s1210_state *st,
if (val != 0)
return -EINVAL;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/*
* We need to read both high and low registers first so we can preserve
* the hysteresis.
*/
ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
if (ret < 0)
- goto error_ret;
+ return ret;
ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
if (ret < 0)
- goto error_ret;
+ return ret;
hysteresis = high_reg_val - low_reg_val;
high_reg_val = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
@@ -615,14 +592,9 @@ static int ad2s1210_set_lot_high_threshold(struct ad2s1210_state *st,
ret = regmap_write(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, high_reg_val);
if (ret < 0)
- goto error_ret;
-
- ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD, low_reg_val);
-
-error_ret:
- mutex_unlock(&st->lock);
+ return ret;
- return ret;
+ return regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD, low_reg_val);
}
static int ad2s1210_get_lot_low_threshold(struct ad2s1210_state *st,
@@ -631,16 +603,13 @@ static int ad2s1210_get_lot_low_threshold(struct ad2s1210_state *st,
unsigned int high_reg_val, low_reg_val;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
+
ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, &high_reg_val);
if (ret < 0)
- goto error_ret;
+ return ret;
ret = regmap_read(st->regmap, AD2S1210_REG_LOT_LOW_THRD, &low_reg_val);
-
-error_ret:
- mutex_unlock(&st->lock);
-
if (ret < 0)
return ret;
@@ -663,18 +632,14 @@ static int ad2s1210_set_lot_low_threshold(struct ad2s1210_state *st,
hysteresis = val2 / ad2s1210_lot_threshold_urad_per_lsb[st->resolution];
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
+
ret = regmap_read(st->regmap, AD2S1210_REG_LOT_HIGH_THRD, ®_val);
if (ret < 0)
- goto error_ret;
+ return ret;
- ret = regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD,
+ return regmap_write(st->regmap, AD2S1210_REG_LOT_LOW_THRD,
reg_val - hysteresis);
-
-error_ret:
- mutex_unlock(&st->lock);
-
- return ret;
}
static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val)
@@ -682,31 +647,23 @@ static int ad2s1210_get_excitation_frequency(struct ad2s1210_state *st, int *val
unsigned int reg_val;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
+
ret = regmap_read(st->regmap, AD2S1210_REG_EXCIT_FREQ, ®_val);
if (ret < 0)
- goto error_ret;
+ return ret;
*val = reg_val * st->clkin_hz / (1 << 15);
- ret = IIO_VAL_INT;
-
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
+ return IIO_VAL_INT;
}
static int ad2s1210_set_excitation_frequency(struct ad2s1210_state *st, int val)
{
- int ret;
-
if (val < AD2S1210_MIN_EXCIT || val > AD2S1210_MAX_EXCIT)
return -EINVAL;
- mutex_lock(&st->lock);
- ret = ad2s1210_reinit_excitation_frequency(st, val);
- mutex_unlock(&st->lock);
-
- return ret;
+ guard(mutex)(&st->lock);
+ return ad2s1210_reinit_excitation_frequency(st, val);
}
static const int ad2s1210_velocity_scale[] = {
@@ -982,10 +939,8 @@ static ssize_t event_attr_voltage_reg_show(struct device *dev,
unsigned int value;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = regmap_read(st->regmap, iattr->address, &value);
- mutex_unlock(&st->lock);
-
if (ret < 0)
return ret;
@@ -1005,11 +960,9 @@ static ssize_t event_attr_voltage_reg_store(struct device *dev,
if (ret)
return -EINVAL;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
ret = regmap_write(st->regmap, iattr->address,
data / THRESHOLD_MILLIVOLT_PER_LSB);
- mutex_unlock(&st->lock);
-
if (ret < 0)
return ret;
@@ -1083,7 +1036,7 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
unsigned int data;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
/* Use default config register value plus resolution from devicetree. */
data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1);
@@ -1093,13 +1046,9 @@ static int ad2s1210_initial(struct ad2s1210_state *st)
ret = regmap_write(st->regmap, AD2S1210_REG_CONTROL, data);
if (ret < 0)
- goto error_ret;
-
- ret = ad2s1210_reinit_excitation_frequency(st, AD2S1210_DEF_EXCIT);
+ return ret;
-error_ret:
- mutex_unlock(&st->lock);
- return ret;
+ return ad2s1210_reinit_excitation_frequency(st, AD2S1210_DEF_EXCIT);
}
static int ad2s1210_read_label(struct iio_dev *indio_dev,
@@ -1243,18 +1192,13 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
unsigned int *readval)
{
struct ad2s1210_state *st = iio_priv(indio_dev);
- int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
if (readval)
- ret = regmap_read(st->regmap, reg, readval);
- else
- ret = regmap_write(st->regmap, reg, writeval);
-
- mutex_unlock(&st->lock);
+ return regmap_read(st->regmap, reg, readval);
- return ret;
+ return regmap_write(st->regmap, reg, writeval);
}
static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
@@ -1265,7 +1209,7 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
size_t chan = 0;
int ret;
- mutex_lock(&st->lock);
+ guard(mutex)(&st->lock);
memset(&st->scan, 0, sizeof(st->scan));
gpiod_set_value(st->sample_gpio, 1);
@@ -1299,7 +1243,6 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
error_ret:
gpiod_set_value(st->sample_gpio, 0);
- mutex_unlock(&st->lock);
iio_trigger_notify_done(indio_dev->trig);
return IRQ_HANDLED;
--
2.42.0
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex)
2023-10-06 0:50 ` [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex) David Lechner
@ 2023-10-10 16:17 ` Jonathan Cameron
2023-10-10 17:40 ` David Lechner
0 siblings, 1 reply; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 16:17 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Thu, 5 Oct 2023 19:50:34 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can simplify the code and get rid of most of the gotos by using
> guard(mutex) from cleanup.h.
You could consider scoped_guard() for a few cases in here, but perhaps
it's better to be consistent and always use the guard() version.
There is a small timing question wrt to the gpio manipulation inline.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>
> v4 changes: New patch in v4.
>
> drivers/staging/iio/resolver/ad2s1210.c | 157 ++++++++++----------------------
> 1 file changed, 50 insertions(+), 107 deletions(-)
>
> diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> index c4e1bc22e8b0..c4e0ffa92dc2 100644
> --- a/drivers/staging/iio/resolver/ad2s1210.c
> +++ b/drivers/staging/iio/resolver/ad2s1210.c
> @@ -47,6 +47,7 @@
>
> #include <linux/bitfield.h>
> #include <linux/bits.h>
> +#include <linux/cleanup.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> @@ -404,11 +405,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> s64 timestamp;
> int ret;
>
> - mutex_lock(&st->lock);
> + guard(mutex)(&st->lock);
> +
> gpiod_set_value(st->sample_gpio, 1);
> timestamp = iio_get_time_ns(indio_dev);
> /* delay (6 * tck + 20) nano seconds */
> udelay(1);
> + gpiod_set_value(st->sample_gpio, 0);
>
> switch (chan->type) {
> case IIO_ANGL:
> @@ -418,14 +421,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> ret = ad2s1210_set_mode(st, MOD_VEL);
> break;
> default:
> - ret = -EINVAL;
> - break;
> + return -EINVAL;
> }
> if (ret < 0)
> - goto error_ret;
> + return ret;
> ret = spi_read(st->sdev, &st->sample, 3);
> if (ret < 0)
> - goto error_ret;
> + return ret;
>
> switch (chan->type) {
> case IIO_ANGL:
> @@ -437,17 +439,11 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> ret = IIO_VAL_INT;
> break;
> default:
> - ret = -EINVAL;
> - break;
> + return -EINVAL;
> }
>
> ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
>
> -error_ret:
> - gpiod_set_value(st->sample_gpio, 0);
> - /* delay (2 * tck + 20) nano seconds */
> - udelay(1);
Dropping this delay isn't obviously safe (though it probably is given stuff done before we exit).
I assume there are no rules on holding the gpio down for the register read.
If nothing else I think the patch description needs to made an argument for why it is fine.
> - mutex_unlock(&st->lock);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex)
2023-10-10 16:17 ` Jonathan Cameron
@ 2023-10-10 17:40 ` David Lechner
2023-10-10 17:46 ` Jonathan Cameron
0 siblings, 1 reply; 37+ messages in thread
From: David Lechner @ 2023-10-10 17:40 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Tue, Oct 10, 2023 at 11:17 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 5 Oct 2023 19:50:34 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > We can simplify the code and get rid of most of the gotos by using
> > guard(mutex) from cleanup.h.
> You could consider scoped_guard() for a few cases in here, but perhaps
> it's better to be consistent and always use the guard() version.
Yes, there it doesn't look like there are any cases where there is any
long-running operation that could be done after unlocking the mutex,
so I went with the simpler approach everywhere.
>
> There is a small timing question wrt to the gpio manipulation inline.
>
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >
> > v4 changes: New patch in v4.
> >
> > drivers/staging/iio/resolver/ad2s1210.c | 157 ++++++++++----------------------
> > 1 file changed, 50 insertions(+), 107 deletions(-)
> >
> > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > index c4e1bc22e8b0..c4e0ffa92dc2 100644
> > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > @@ -47,6 +47,7 @@
> >
> > #include <linux/bitfield.h>
> > #include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > #include <linux/clk.h>
> > #include <linux/delay.h>
> > #include <linux/device.h>
> > @@ -404,11 +405,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> > s64 timestamp;
> > int ret;
> >
> > - mutex_lock(&st->lock);
> > + guard(mutex)(&st->lock);
> > +
> > gpiod_set_value(st->sample_gpio, 1);
> > timestamp = iio_get_time_ns(indio_dev);
> > /* delay (6 * tck + 20) nano seconds */
> > udelay(1);
> > + gpiod_set_value(st->sample_gpio, 0);
> >
> > switch (chan->type) {
> > case IIO_ANGL:
> > @@ -418,14 +421,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> > ret = ad2s1210_set_mode(st, MOD_VEL);
> > break;
> > default:
> > - ret = -EINVAL;
> > - break;
> > + return -EINVAL;
> > }
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> > ret = spi_read(st->sdev, &st->sample, 3);
> > if (ret < 0)
> > - goto error_ret;
> > + return ret;
> >
> > switch (chan->type) {
> > case IIO_ANGL:
> > @@ -437,17 +439,11 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> > ret = IIO_VAL_INT;
> > break;
> > default:
> > - ret = -EINVAL;
> > - break;
> > + return -EINVAL;
> > }
> >
> > ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
> >
> > -error_ret:
> > - gpiod_set_value(st->sample_gpio, 0);
> > - /* delay (2 * tck + 20) nano seconds */
> > - udelay(1);
>
> Dropping this delay isn't obviously safe (though it probably is given stuff done before we exit).
> I assume there are no rules on holding the gpio down for the register read.
Correct. The SAMPLE gpio only needs to be held for a short time (~350
nanoseconds) to latch in the current values, then it doesn't matter
when it is released. (Figure 35 in datasheet)
>
> If nothing else I think the patch description needs to made an argument for why it is fine.
The longest possible delay needed after releasing the SAMPLE line
before reasserting is ~350 nanoseconds. Is there a rule of thumb for
deciding when there are enough instructions that no processor could
execute them faster than this vs. when we should add an explicit
delay?
I think I will consider adding a patch in the next round to refactor
the SAMPLE toggle to a separate function so we can be sure it is
handled the same in all cases.
>
> > - mutex_unlock(&st->lock);
> > return ret;
> > }
> >
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v4 17/17] staging: iio: resolver: ad2s1210: simplify code with guard(mutex)
2023-10-10 17:40 ` David Lechner
@ 2023-10-10 17:46 ` Jonathan Cameron
0 siblings, 0 replies; 37+ messages in thread
From: Jonathan Cameron @ 2023-10-10 17:46 UTC (permalink / raw)
To: David Lechner
Cc: linux-iio, linux-staging, Michael Hennerich, Nuno Sá,
Axel Haslam, Philip Molloy, linux-kernel
On Tue, 10 Oct 2023 12:40:03 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On Tue, Oct 10, 2023 at 11:17 AM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 5 Oct 2023 19:50:34 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >
> > > We can simplify the code and get rid of most of the gotos by using
> > > guard(mutex) from cleanup.h.
> > You could consider scoped_guard() for a few cases in here, but perhaps
> > it's better to be consistent and always use the guard() version.
>
> Yes, there it doesn't look like there are any cases where there is any
> long-running operation that could be done after unlocking the mutex,
> so I went with the simpler approach everywhere.
>
> >
> > There is a small timing question wrt to the gpio manipulation inline.
> >
> > >
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >
> > > v4 changes: New patch in v4.
> > >
> > > drivers/staging/iio/resolver/ad2s1210.c | 157 ++++++++++----------------------
> > > 1 file changed, 50 insertions(+), 107 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c
> > > index c4e1bc22e8b0..c4e0ffa92dc2 100644
> > > --- a/drivers/staging/iio/resolver/ad2s1210.c
> > > +++ b/drivers/staging/iio/resolver/ad2s1210.c
> > > @@ -47,6 +47,7 @@
> > >
> > > #include <linux/bitfield.h>
> > > #include <linux/bits.h>
> > > +#include <linux/cleanup.h>
> > > #include <linux/clk.h>
> > > #include <linux/delay.h>
> > > #include <linux/device.h>
> > > @@ -404,11 +405,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> > > s64 timestamp;
> > > int ret;
> > >
> > > - mutex_lock(&st->lock);
> > > + guard(mutex)(&st->lock);
> > > +
> > > gpiod_set_value(st->sample_gpio, 1);
> > > timestamp = iio_get_time_ns(indio_dev);
> > > /* delay (6 * tck + 20) nano seconds */
> > > udelay(1);
> > > + gpiod_set_value(st->sample_gpio, 0);
> > >
> > > switch (chan->type) {
> > > case IIO_ANGL:
> > > @@ -418,14 +421,13 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> > > ret = ad2s1210_set_mode(st, MOD_VEL);
> > > break;
> > > default:
> > > - ret = -EINVAL;
> > > - break;
> > > + return -EINVAL;
> > > }
> > > if (ret < 0)
> > > - goto error_ret;
> > > + return ret;
> > > ret = spi_read(st->sdev, &st->sample, 3);
> > > if (ret < 0)
> > > - goto error_ret;
> > > + return ret;
> > >
> > > switch (chan->type) {
> > > case IIO_ANGL:
> > > @@ -437,17 +439,11 @@ static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
> > > ret = IIO_VAL_INT;
> > > break;
> > > default:
> > > - ret = -EINVAL;
> > > - break;
> > > + return -EINVAL;
> > > }
> > >
> > > ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
> > >
> > > -error_ret:
> > > - gpiod_set_value(st->sample_gpio, 0);
> > > - /* delay (2 * tck + 20) nano seconds */
> > > - udelay(1);
> >
> > Dropping this delay isn't obviously safe (though it probably is given stuff done before we exit).
> > I assume there are no rules on holding the gpio down for the register read.
>
> Correct. The SAMPLE gpio only needs to be held for a short time (~350
> nanoseconds) to latch in the current values, then it doesn't matter
> when it is released. (Figure 35 in datasheet)
>
> >
> > If nothing else I think the patch description needs to made an argument for why it is fine.
>
> The longest possible delay needed after releasing the SAMPLE line
> before reasserting is ~350 nanoseconds. Is there a rule of thumb for
> deciding when there are enough instructions that no processor could
> execute them faster than this vs. when we should add an explicit
> delay?
Almost always use an explicit delay as then we don't have to think about it.
>
> I think I will consider adding a patch in the next round to refactor
> the SAMPLE toggle to a separate function so we can be sure it is
> handled the same in all cases.
Sounds good.
Jonathan
>
> >
> > > - mutex_unlock(&st->lock);
> > > return ret;
> > > }
> > >
^ permalink raw reply [flat|nested] 37+ messages in thread