| Message ID | 20230929-ad2s1210-mainline-v3-0-fa4364281745@baylibre.com |
|---|---|
| Headers | show |
| Series | iio: resolver: move ad2s1210 out of staging | expand |
On Fri, Sep 29, 2023 at 12:25 PM David Lechner <dlechner@baylibre.com> wrote: > > From: David Lechner <david@lechnology.com> > Ugh, an automated tool picked up my personal email on this for some reason. This extra From: line can be dropped from any patches that get picked up on this round. I will make sure to double-check next time.
On Fri, Sep 29, 2023 at 12:25 PM 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> > --- ... > +static int ad2s1210_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, > + int *length, long mask) > +{ > + static const int hysteresis_available[] = { 0, 1 }; This is basically an enable/disable. Should the 1 value be changed to the appropriate radians value since this is hysteresis on the position (angle) channel? > + > + switch (mask) { > + case IIO_CHAN_INFO_HYSTERESIS: > + switch (chan->type) { > + case IIO_ANGL: > + *vals = hysteresis_available; > + *type = IIO_VAL_INT; > + *length = ARRAY_SIZE(hysteresis_available); > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} >
On 9/29/23 12:23 PM, David Lechner wrote: > From: David Lechner <david@lechnology.com> > > v3 changes: > > * Added description of A0/A1 lines in DT bindings. > * Added power supply regulators to DT bindings. > * Dropped "staging: iio: Documentation: document IIO resolver AD2S1210 > sysfs attributes" (these attributes are being removed instead). > * Dropped applied patches: > * "staging: iio: resolver: ad2s1210: fix ad2s1210_show_fault" > * "iio: adc: MCP3564: fix the static checker warning" > * Split "staging: iio: resolver: ad2s1210: fix probe" into multiple patches. > * Moved sorting imports to separate patch. > * Renamed fclkin to clkin_hz. > * Added __be16 sample field to state struct for reading raw samples. > * Split out new function ad2s1210_single_conversion() from > ad2s1210_read_raw(). > * Split out new ad2s1210_get_hysteresis() and ad2s1210_set_hysteresis() > functions. > * Fixed multi-line comment style. > * Added notes about soft reset not resetting config registers. > * Made use of FIELD_PREP() macro. > * Added more explanation to regmap commit message. > * Removed datasheet names from channel specs. > * Replaced "staging: iio: resolver: ad2s1210: rename fexcit attribute" > with "staging: iio: resolver: ad2s1210: convert fexcit to channel > attribute". > * Replaced "staging: iio: resolver: ad2s1210: add phase_lock_range > attributes" with "staging: iio: resolver: ad2s1210: add phase lock > range support" > * Added additional patches to convert custom device attributes to event > attributes. > * Added patch to add channel label attributes. > > v2 changes: > * Address initial device tree patch feedback > * Drop "iio: sysfs: add IIO_DEVICE_ATTR_NAMED_RW macro" (related cleanups > also dropped for now, will address in a future series if needed) > * Apply improvements as a series of patches to the staging driver. It is > not quite ready for the move out of staging patch yet. > > This series has been tested on actual hardware using a EVAL-AD2S1210 evaluation > board. (Note: not all device tree features have been implemented in the driver > since the eval board doesn't support them out of the box. We plan to add them > later if needed.) > > Most of the questions about dealing with faults from the v2 cover letter > have been addressed. There is still the question about what to do with > the current `fault` attribute (it is the only custom device attribute > remaining from the original staging driver). It was suggested to split it > out into multiple attributes in a subdirectory. Since we now have events > for all of the faults, I'm wondering if this is something that is still needed. > In the current implementation, it is possible to start listening to events, > clear the faults and then read a sample to trigger events for any current > faults so we have a way to get current faults already. > > There is also the matter of clearing faults. Writing the excitation > frequency has a side-effect of clearing the faults, so we could use > that as the reset. Or we could change the current fault attribute to > write-only and rename it. Or is there a better way that I have overlooked? > > Once this last issue is addressed, I think this driver will be ready > for consideration for moving out of staging. > --- > David Lechner (27): > dt-bindings: iio: resolver: add devicetree bindings for ad2s1210 > staging: iio: resolver: ad2s1210: fix use before initialization > staging: iio: resolver: ad2s1210: remove call to spi_setup() > staging: iio: resolver: ad2s1210: check return of ad2s1210_initial() > staging: iio: resolver: ad2s1210: remove spi_set_drvdata() > staging: iio: resolver: ad2s1210: sort imports > staging: iio: resolver: ad2s1210: always use 16-bit value for raw read > staging: iio: resolver: ad2s1210: implement IIO_CHAN_INFO_SCALE > staging: iio: resolver: ad2s1210: use devicetree to get CLKIN rate > staging: iio: resolver: ad2s1210: use regmap for config registers > staging: iio: resolver: ad2s1210: add debugfs reg access > staging: iio: resolver: ad2s1210: remove config attribute > staging: iio: resolver: ad2s1210: rework gpios > staging: iio: resolver: ad2s1210: implement hysteresis as channel attr > staging: iio: resolver: ad2s1210: refactor setting excitation frequency > staging: iio: resolver: ad2s1210: read excitation frequency from control register > staging: iio: resolver: ad2s1210: convert fexcit to channel attribute > staging: iio: resolver: ad2s1210: convert resolution to devicetree property > staging: iio: resolver: ad2s1210: add phase lock range support > staging: iio: resolver: ad2s1210: add triggered buffer support > staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs > staging: iio: resolver: ad2s1210: convert LOS threshold to event attr > staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr > staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr > staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs > staging: iio: resolver: ad2s1210: implement fault events > staging: iio: resolver: ad2s1210: add label attribute support > > .../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++ > .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 + > drivers/staging/iio/resolver/Kconfig | 1 + > drivers/staging/iio/resolver/ad2s1210.c | 1583 +++++++++++++++----- > 4 files changed, 1391 insertions(+), 397 deletions(-) > --- > base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec > change-id: 20230925-ad2s1210-mainline-2791ef75e386 > In the end, this is what sysfs looks like: root@analog:~# tree /sys/bus/iio/devices/iio\:device1/ /sys/bus/iio/devices/iio:device1/ ├── buffer │ ├── data_available │ ├── direction │ ├── enable │ ├── length │ └── watermark ├── buffer0 │ ├── data_available │ ├── direction │ ├── enable │ ├── in_angl0_en │ ├── in_angl0_index │ ├── in_angl0_type │ ├── in_anglvel0_en │ ├── in_anglvel0_index │ ├── in_anglvel0_type │ ├── in_timestamp_en │ ├── in_timestamp_index │ ├── in_timestamp_type │ ├── length │ └── watermark ├── current_timestamp_clock ├── dev ├── events │ ├── in_altvoltage0_mag_reset_max │ ├── in_altvoltage0_mag_reset_max_available │ ├── in_altvoltage0_mag_reset_min │ ├── in_altvoltage0_mag_reset_min_available │ ├── in_altvoltage0_mag_value │ ├── in_altvoltage0_mag_value_available │ ├── in_altvoltage0_thresh_falling_value │ ├── in_altvoltage0_thresh_falling_value_available │ ├── in_altvoltage0_thresh_rising_value │ ├── in_altvoltage0_thresh_rising_value_available │ ├── in_angl1_thresh_rising_hysteresis │ ├── in_angl1_thresh_rising_hysteresis_available │ ├── in_angl1_thresh_rising_value │ ├── in_angl1_thresh_rising_value_available │ ├── in_phase0_mag_value │ └── in_phase0_mag_value_available ├── fault ├── in_altvoltage0_label ├── in_altvoltage1_x_label ├── in_altvoltage1_y_label ├── in_angl0_hysteresis ├── in_angl0_hysteresis_available ├── in_angl0_label ├── in_angl0_raw ├── in_angl0_scale ├── in_angl1_label ├── in_anglvel0_label ├── in_anglvel0_raw ├── in_anglvel0_scale ├── in_phase0_label ├── name ├── of_node -> ../../../../../../../../firmware/devicetree/base/axi/spi@e0006000/ad2s1210@0 ├── out_altvoltage0_frequency ├── out_altvoltage0_frequency_available ├── out_altvoltage0_label ├── power │ ├── autosuspend_delay_ms │ ├── control │ ├── runtime_active_time │ ├── runtime_status │ └── runtime_suspended_time ├── scan_elements │ ├── in_angl0_en │ ├── in_angl0_index │ ├── in_angl0_type │ ├── in_anglvel0_en │ ├── in_anglvel0_index │ ├── in_anglvel0_type │ ├── in_timestamp_en │ ├── in_timestamp_index │ └── in_timestamp_type ├── subsystem -> ../../../../../../../../bus/iio ├── trigger │ └── current_trigger ├── uevent └── waiting_for_supplier 8 directories, 72 files And this is what the output of iio_info looks like: (note: iio_info does not currently support events so those attributes are not visible here) iio:device1: ad2s1210 (buffer capable) 9 channels found: angl0: (input, index: 0, format: be:U16/16>>0) 5 channel-specific attributes found: attr 0: hysteresis value: 1 attr 1: hysteresis_available value: 0 1 attr 2: label value: position attr 3: raw value: 12578 attr 4: scale value: 0.000095874 anglvel0: (input, index: 1, format: be:S16/16>>0) 3 channel-specific attributes found: attr 0: label value: velocity attr 1: raw value: 5 attr 2: scale value: 0.023968449 timestamp: (input, index: 2, format: le:S64/64>>0) phase0: (input) 1 channel-specific attributes found: attr 0: label value: synthetic reference altvoltage0: (output) 3 channel-specific attributes found: attr 0: frequency value: 10000 attr 1: frequency_available value: [2000 250 20000] attr 2: label value: excitation angl1: (input) 1 channel-specific attributes found: attr 0: label value: tracking error altvoltage1_y: (input) 1 channel-specific attributes found: attr 0: label value: sine altvoltage0: (input) 1 channel-specific attributes found: attr 0: label value: monitor signal altvoltage1_x: (input) 1 channel-specific attributes found: attr 0: label value: cosine 3 device-specific attributes found: attr 0: current_timestamp_clock value: realtime attr 1: fault value: 0x00 attr 2: waiting_for_supplier value: 0 2 buffer-specific attributes found: attr 0: data_available value: 0 attr 1: direction value: in 1 debug attributes found: debug attr 0: direct_reg_access ERROR: Input/output error (5) Current trigger: trigger0(test)
On Fri, 29 Sep 2023 12:49:42 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Fri, Sep 29, 2023 at 12:25 PM David Lechner <dlechner@baylibre.com> wrote: > > > > From: David Lechner <david@lechnology.com> > > > > Ugh, an automated tool picked up my personal email on this for some > reason. This extra From: line can be dropped from any patches that get > picked up on this round. I will make sure to double-check next time. Seems b4 picks up the second, correct From anyway which is handy ;) Jonathan
On Fri, 29 Sep 2023 12:23:07 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This fixes a use before initialization in ad2s1210_probe(). The > ad2s1210_setup_gpios() function uses st->sdev but it was being called > before this field was initialized. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied to the togreg banch of iio.git and pushed out as testing for 0-day to poke at it. I didn't pull this out as a fix to upstream quicker because it would make a mess of the rest of applying the rest of the series. Maybe we want to consider backporting some of these at somepoint. Jonathan > --- > > v3 changes: > * This is a new patch split out from "staging: iio: resolver: ad2s1210: > fix probe" > > drivers/staging/iio/resolver/ad2s1210.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index f695ca0547e4..3f08b59f4e19 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -658,9 +658,6 @@ static int ad2s1210_probe(struct spi_device *spi) > if (!indio_dev) > return -ENOMEM; > st = iio_priv(indio_dev); > - ret = ad2s1210_setup_gpios(st); > - if (ret < 0) > - return ret; > > spi_set_drvdata(spi, indio_dev); > > @@ -671,6 +668,10 @@ static int ad2s1210_probe(struct spi_device *spi) > st->resolution = 12; > st->fexcit = AD2S1210_DEF_EXCIT; > > + ret = ad2s1210_setup_gpios(st); > + if (ret < 0) > + return ret; > + > indio_dev->info = &ad2s1210_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = ad2s1210_channels; >
On Fri, 29 Sep 2023 12:23:08 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This removes the call to spi_setup() in the ad2s1210 driver. > > Setting MODE_3 was incorrect. It should be MODE_1 but we can let the > device tree select this and avoid the need to call spi_setup(). > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied and pushed out as testing for all the normal reasons. Thanks, Jonathan > --- > > v3 changes: > * This is a new patch split out from "staging: iio: resolver: ad2s1210: > fix probe" > > drivers/staging/iio/resolver/ad2s1210.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 3f08b59f4e19..8fde08887f7f 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -683,8 +683,6 @@ static int ad2s1210_probe(struct spi_device *spi) > return ret; > > st->fclkin = spi->max_speed_hz; > - spi->mode = SPI_MODE_3; > - spi_setup(spi); > ad2s1210_initial(st); > > return 0; >
On Fri, 29 Sep 2023 12:23:09 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This adds a check to the return value of ad2s1210_initial() since it > can fail. The call is also moved before devm_iio_device_register() so > that we don't have to unregister the device if it fails. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied > --- > > v3 changes: > * This is a new patch split out from "staging: iio: resolver: ad2s1210: > fix probe" > > drivers/staging/iio/resolver/ad2s1210.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 8fde08887f7f..b5e071d7c5fd 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -672,6 +672,10 @@ static int ad2s1210_probe(struct spi_device *spi) > if (ret < 0) > return ret; > > + ret = ad2s1210_initial(st); > + if (ret < 0) > + return ret; > + > indio_dev->info = &ad2s1210_info; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = ad2s1210_channels; > @@ -683,7 +687,6 @@ static int ad2s1210_probe(struct spi_device *spi) > return ret; > > st->fclkin = spi->max_speed_hz; > - ad2s1210_initial(st); > > return 0; > } >
On Fri, 29 Sep 2023 12:23:10 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > Since we never call spi_get_drvdata(), we can remove spi_set_drvdata(). > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied. > --- > > v3 changes: > * This is a new patch split out from "staging: iio: resolver: ad2s1210: > fix probe" > > drivers/staging/iio/resolver/ad2s1210.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index b5e071d7c5fd..28015322f562 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -659,8 +659,6 @@ static int ad2s1210_probe(struct spi_device *spi) > return -ENOMEM; > st = iio_priv(indio_dev); > > - spi_set_drvdata(spi, indio_dev); > - > mutex_init(&st->lock); > st->sdev = spi; > st->hysteresis = true; >
On Fri, 29 Sep 2023 12:23:11 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > There are quite a few imports and we will be adding more so it will > make it easier to read if they are sorted. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied > --- > > v3 changes: > * This is a new patch split out from "staging: iio: resolver: ad2s1210: > use devicetree to get fclkin" > > drivers/staging/iio/resolver/ad2s1210.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 28015322f562..832f86bf15e5 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -4,16 +4,16 @@ > * > * Copyright (c) 2010-2010 Analog Devices Inc. > */ > -#include <linux/types.h> > -#include <linux/mutex.h> > +#include <linux/delay.h> > #include <linux/device.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/of.h> > -#include <linux/spi/spi.h> > #include <linux/slab.h> > +#include <linux/spi/spi.h> > #include <linux/sysfs.h> > -#include <linux/delay.h> > -#include <linux/gpio/consumer.h> > -#include <linux/module.h> > +#include <linux/types.h> > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> >
On Fri, 29 Sep 2023 12:23:12 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This removes the special handling for resolutions lower than 16 bits. > This will allow us to use a fixed scale independent of the resolution. > > A new sample field is added to store the raw data instead of reusing > the config mode rx buffer so that we don't have to do a cast or worry > about unaligned access. > > Also, for the record, according to the datasheet, the logic for the > special handling based on hysteresis that was removed was incorrect. > > Signed-off-by: David Lechner <dlechner@baylibre.com> LGTM Applied. Thanks, Jonathan > --- > > v3 changes: > * Added __be16 sample field to state struct and use instead of rx buffer. > > drivers/staging/iio/resolver/ad2s1210.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 832f86bf15e5..f9774dff2df4 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -95,7 +95,11 @@ struct ad2s1210_state { > bool hysteresis; > u8 resolution; > enum ad2s1210_mode mode; > - u8 rx[2] __aligned(IIO_DMA_MINALIGN); > + /** For reading raw sample value via SPI. */ > + __be16 sample __aligned(IIO_DMA_MINALIGN); > + /** SPI transmit buffer. */ > + u8 rx[2]; > + /** SPI receive buffer. */ > u8 tx[2]; > }; > > @@ -464,10 +468,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > long m) > { > struct ad2s1210_state *st = iio_priv(indio_dev); > - u16 negative; > int ret = 0; > - u16 pos; > - s16 vel; > > mutex_lock(&st->lock); > gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0); > @@ -487,26 +488,17 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > } > if (ret < 0) > goto error_ret; > - ret = spi_read(st->sdev, st->rx, 2); > + ret = spi_read(st->sdev, &st->sample, 2); > if (ret < 0) > goto error_ret; > > switch (chan->type) { > case IIO_ANGL: > - pos = be16_to_cpup((__be16 *)st->rx); > - if (st->hysteresis) > - pos >>= 16 - st->resolution; > - *val = pos; > + *val = be16_to_cpu(st->sample); > ret = IIO_VAL_INT; > break; > case IIO_ANGL_VEL: > - vel = be16_to_cpup((__be16 *)st->rx); > - vel >>= 16 - st->resolution; > - if (vel & 0x8000) { > - negative = (0xffff >> st->resolution) << st->resolution; > - vel |= negative; > - } > - *val = vel; > + *val = (s16)be16_to_cpu(st->sample); > ret = IIO_VAL_INT; > break; > default: >
On Fri, 29 Sep 2023 12:23:13 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This adds an implementation of IIO_CHAN_INFO_SCALE to the ad2s1210 > resolver driver. This allows userspace to get the scale factor for the > raw values. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied Thanks, > --- > > v3 changes: > * Split ad2s1210_read_raw() into two functions to reduce complexity. > * Use early return instead of break in switch statements. > > drivers/staging/iio/resolver/ad2s1210.c | 53 ++++++++++++++++++++++++++++----- > 1 file changed, 45 insertions(+), 8 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index f9774dff2df4..a710598a64f0 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -461,13 +461,10 @@ static ssize_t ad2s1210_store_reg(struct device *dev, > return ret < 0 ? ret : len; > } > > -static int ad2s1210_read_raw(struct iio_dev *indio_dev, > - struct iio_chan_spec const *chan, > - int *val, > - int *val2, > - long m) > +static int ad2s1210_single_conversion(struct ad2s1210_state *st, > + struct iio_chan_spec const *chan, > + int *val) > { > - struct ad2s1210_state *st = iio_priv(indio_dev); > int ret = 0; > > mutex_lock(&st->lock); > @@ -514,6 +511,44 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > return ret; > } > > +static const int ad2s1210_velocity_scale[] = { > + 17089132, /* 8.192MHz / (2*pi * 2500 / 2^15) */ > + 42722830, /* 8.192MHz / (2*pi * 1000 / 2^15) */ > + 85445659, /* 8.192MHz / (2*pi * 500 / 2^15) */ > + 341782638, /* 8.192MHz / (2*pi * 125 / 2^15) */ > +}; > + > +static int ad2s1210_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, > + int *val2, > + long mask) > +{ > + struct ad2s1210_state *st = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + return ad2s1210_single_conversion(st, chan, val); > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { > + case IIO_ANGL: > + /* approx 0.3 arc min converted to radians */ > + *val = 0; > + *val2 = 95874; > + return IIO_VAL_INT_PLUS_NANO; > + case IIO_ANGL_VEL: > + *val = st->fclkin; > + *val2 = ad2s1210_velocity_scale[st->resolution]; > + return IIO_VAL_FRACTIONAL; > + default: > + return -EINVAL; > + } > + > + default: > + return -EINVAL; > + } > +} > + > static IIO_DEVICE_ATTR(fclkin, 0644, > ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0); > static IIO_DEVICE_ATTR(fexcit, 0644, > @@ -552,12 +587,14 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > .type = IIO_ANGL, > .indexed = 1, > .channel = 0, > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > }, { > .type = IIO_ANGL_VEL, > .indexed = 1, > .channel = 0, > - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > } > }; > >
On Fri, 29 Sep 2023 12:23:14 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This removes the fclkin sysfs attribute and replaces it with getting > the CLKIN clock rate using the clk subsystem (i.e. from the devicetree). > > CLKIN comes from an external oscillator that is connected directly to > the AD2S1210 chip, so users of the sysfs attributes should not need to > be concerned with this. > > The fclkin field (the datasheet name) is renamed to clkin_hz to be more > obvious that it is a frequency in Hz. > > Signed-off-by: David Lechner <dlechner@baylibre.com> LGTM Applied J > --- > > v3 changes: > * Don't sort imports in this patch. > * Renamed fexcit to clkin_hz. > * Fixed ad2s1210_setup_clocks() being called in an earlier patch. > > drivers/staging/iio/resolver/Kconfig | 1 + > drivers/staging/iio/resolver/ad2s1210.c | 81 ++++++++++++--------------------- > 2 files changed, 30 insertions(+), 52 deletions(-) > > diff --git a/drivers/staging/iio/resolver/Kconfig b/drivers/staging/iio/resolver/Kconfig > index 6d1e2622e0b0..bebb35822c9e 100644 > --- a/drivers/staging/iio/resolver/Kconfig > +++ b/drivers/staging/iio/resolver/Kconfig > @@ -7,6 +7,7 @@ menu "Resolver to digital converters" > config AD2S1210 > tristate "Analog Devices ad2s1210 driver" > depends on SPI > + depends on COMMON_CLK > depends on GPIOLIB || COMPILE_TEST > help > Say yes here to build support for Analog Devices spi resolver > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index a710598a64f0..c8723b6f3a3b 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -3,7 +3,9 @@ > * ad2s1210.c support for the ADI Resolver to Digital Converters: AD2S1210 > * > * Copyright (c) 2010-2010 Analog Devices Inc. > + * Copyright (c) 2023 BayLibre, SAS > */ > +#include <linux/clk.h> > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/gpio/consumer.h> > @@ -90,7 +92,8 @@ struct ad2s1210_state { > struct mutex lock; > struct spi_device *sdev; > struct gpio_desc *gpios[5]; > - unsigned int fclkin; > + /** The external oscillator frequency in Hz. */ > + unsigned long clkin_hz; > unsigned int fexcit; > bool hysteresis; > u8 resolution; > @@ -165,7 +168,7 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st) > int ret; > unsigned char fcw; > > - fcw = (unsigned char)(st->fexcit * (1 << 15) / st->fclkin); > + fcw = (unsigned char)(st->fexcit * (1 << 15) / st->clkin_hz); > if (fcw < AD2S1210_MIN_FCW || fcw > AD2S1210_MAX_FCW) { > dev_err(&st->sdev->dev, "ad2s1210: FCW out of range\n"); > return -ERANGE; > @@ -201,45 +204,6 @@ static inline int ad2s1210_soft_reset(struct ad2s1210_state *st) > return ad2s1210_config_write(st, 0x0); > } > > -static ssize_t ad2s1210_show_fclkin(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > - > - return sprintf(buf, "%u\n", st->fclkin); > -} > - > -static ssize_t ad2s1210_store_fclkin(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 fclkin; > - int ret; > - > - ret = kstrtouint(buf, 10, &fclkin); > - if (ret) > - return ret; > - if (fclkin < AD2S1210_MIN_CLKIN || fclkin > AD2S1210_MAX_CLKIN) { > - dev_err(dev, "ad2s1210: fclkin out of range\n"); > - return -EINVAL; > - } > - > - mutex_lock(&st->lock); > - st->fclkin = fclkin; > - > - ret = ad2s1210_update_frequency_control_word(st); > - if (ret < 0) > - goto error_ret; > - ret = ad2s1210_soft_reset(st); > -error_ret: > - mutex_unlock(&st->lock); > - > - return ret < 0 ? ret : len; > -} > - > static ssize_t ad2s1210_show_fexcit(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -537,7 +501,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > *val2 = 95874; > return IIO_VAL_INT_PLUS_NANO; > case IIO_ANGL_VEL: > - *val = st->fclkin; > + *val = st->clkin_hz; > *val2 = ad2s1210_velocity_scale[st->resolution]; > return IIO_VAL_FRACTIONAL; > default: > @@ -549,8 +513,6 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > } > } > > -static IIO_DEVICE_ATTR(fclkin, 0644, > - ad2s1210_show_fclkin, ad2s1210_store_fclkin, 0); > static IIO_DEVICE_ATTR(fexcit, 0644, > ad2s1210_show_fexcit, ad2s1210_store_fexcit, 0); > static IIO_DEVICE_ATTR(control, 0644, > @@ -599,7 +561,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > }; > > static struct attribute *ad2s1210_attributes[] = { > - &iio_dev_attr_fclkin.dev_attr.attr, > &iio_dev_attr_fexcit.dev_attr.attr, > &iio_dev_attr_control.dev_attr.attr, > &iio_dev_attr_bits.dev_attr.attr, > @@ -657,6 +618,24 @@ static const struct iio_info ad2s1210_info = { > .attrs = &ad2s1210_attribute_group, > }; > > +static int ad2s1210_setup_clocks(struct ad2s1210_state *st) > +{ > + struct device *dev = &st->sdev->dev; > + struct clk *clk; > + > + clk = devm_clk_get_enabled(dev, NULL); > + if (IS_ERR(clk)) > + return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n"); > + > + st->clkin_hz = clk_get_rate(clk); > + if (st->clkin_hz < AD2S1210_MIN_CLKIN || st->clkin_hz > AD2S1210_MAX_CLKIN) > + return dev_err_probe(dev, -EINVAL, > + "clock frequency out of range: %lu\n", > + st->clkin_hz); > + > + return 0; > +} > + > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > struct spi_device *spi = st->sdev; > @@ -695,6 +674,10 @@ static int ad2s1210_probe(struct spi_device *spi) > st->resolution = 12; > st->fexcit = AD2S1210_DEF_EXCIT; > > + ret = ad2s1210_setup_clocks(st); > + if (ret < 0) > + return ret; > + > ret = ad2s1210_setup_gpios(st); > if (ret < 0) > return ret; > @@ -709,13 +692,7 @@ 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_device_register(&spi->dev, indio_dev); > - if (ret) > - return ret; > - > - st->fclkin = spi->max_speed_hz; > - > - return 0; > + return devm_iio_device_register(&spi->dev, indio_dev); > } > > static const struct of_device_id ad2s1210_of_match[] = { >
On Fri, 29 Sep 2023 12:23:15 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This makes use of the regmap API to read and write the configuration > registers. This simplifies code quite a bit and makes it safer > (previously, it was easy to write a bad value to the config registers > which causes the chip to lock up and need to be reset). > > This chip has multiple modes of operation. In normal mode, we do not use > regmap since there is no addressing - data is just bitshifted out during > the SPI read. In config mode, we use regmap since it requires writing > the address (with read/write flag) before reading and writing. > > We don't use the lock provided by the regmap because we need to also > synchronize with the normal mode SPI reads and with the various GPIOs. > > There is also a quirk when reading registers (other than the fault > register). If the address/data bit is set in the value read, then it > indicates there is a configuration parity error and the data is not > valid. Previously, this was checked in a few places, but not > consistently. Now, we always check it in the regmap read function. > > Signed-off-by: David Lechner <dlechner@baylibre.com> This was a complex change, so I'm partly relying on the fact it clearly works after the change to be sure it is correct ;) Anyhow, code is much cleaner and probably right, so applied to the togreg branch of iio.git and pushed out as testing for all the normal reasons. Thanks, Jonathan
On Fri, 29 Sep 2023 12:23:16 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This add an implementation of debugfs_reg_access for the AD2S1210 > driver. > > Signed-off-by: David Lechner <dlechner@baylibre.com> I never really care if a driver implements this or not. However, if you find it useful that's fine, so applied. Thanks, Jonathan > --- > > v3 changes: None > > drivers/staging/iio/resolver/ad2s1210.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 0663a51d04ad..31415fbb6384 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -614,9 +614,29 @@ static int ad2s1210_initial(struct ad2s1210_state *st) > return ret; > } > > +static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev, > + unsigned int reg, unsigned int writeval, > + unsigned int *readval) > +{ > + struct ad2s1210_state *st = iio_priv(indio_dev); > + int ret; > + > + mutex_lock(&st->lock); > + > + if (readval) > + ret = regmap_read(st->regmap, reg, readval); > + else > + ret = regmap_write(st->regmap, reg, writeval); > + > + mutex_unlock(&st->lock); > + > + return ret; > +} > + > static const struct iio_info ad2s1210_info = { > .read_raw = ad2s1210_read_raw, > .attrs = &ad2s1210_attribute_group, > + .debugfs_reg_access = &ad2s1210_debugfs_reg_access, > }; > > static int ad2s1210_setup_clocks(struct ad2s1210_state *st) >
On Fri, 29 Sep 2023 12:23:17 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This removes the config register sysfs attribute. > > Writing to the config register directly can be dangerous and userspace > should not need to have to know the register layout. This register > can still be accessed though debugfs if needed. > > We can add new attributes to set specific flags in the config register > in the future if needed (e.g. `enable_hysterisis` and > `phase_lock_range`). > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied.
On Fri, 29 Sep 2023 12:23:18 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > - Remove "adi," prefix from gpio names. > - Sample gpio is now expected to be active low. > - Convert A0 and A1 gpios to "mode-gpios" gpio array. > - Convert RES0 and RES1 gpios to "resolution-gpios" gpio array. > - Remove extraneous lookup tables. > - Remove unused mode field from state struct. > - Swap argument order of ad2s1210_set_mode() while we are touching this. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied, Thanks, Jonathan > --- > > v3 changes: > * Fixed multiline comment style. > > drivers/staging/iio/resolver/ad2s1210.c | 164 +++++++++++++++++--------------- > 1 file changed, 85 insertions(+), 79 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 2b9377447f6a..0ec3598b600a 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -58,39 +58,21 @@ > #define AD2S1210_DEF_EXCIT 10000 > > enum ad2s1210_mode { > - MOD_POS = 0, > - MOD_VEL, > - MOD_CONFIG, > - MOD_RESERVED, > + MOD_POS = 0b00, > + MOD_VEL = 0b01, > + MOD_RESERVED = 0b10, > + MOD_CONFIG = 0b11, > }; > > -enum ad2s1210_gpios { > - AD2S1210_SAMPLE, > - AD2S1210_A0, > - AD2S1210_A1, > - AD2S1210_RES0, > - AD2S1210_RES1, > -}; > - > -struct ad2s1210_gpio { > - const char *name; > - unsigned long flags; > -}; > - > -static const struct ad2s1210_gpio gpios[] = { > - [AD2S1210_SAMPLE] = { .name = "adi,sample", .flags = GPIOD_OUT_LOW }, > - [AD2S1210_A0] = { .name = "adi,a0", .flags = GPIOD_OUT_LOW }, > - [AD2S1210_A1] = { .name = "adi,a1", .flags = GPIOD_OUT_LOW }, > - [AD2S1210_RES0] = { .name = "adi,res0", .flags = GPIOD_OUT_LOW }, > - [AD2S1210_RES1] = { .name = "adi,res1", .flags = GPIOD_OUT_LOW }, > -}; > - > -static const unsigned int ad2s1210_resolution_value[] = { 10, 12, 14, 16 }; > - > struct ad2s1210_state { > struct mutex lock; > struct spi_device *sdev; > - struct gpio_desc *gpios[5]; > + /** GPIO pin connected to SAMPLE line. */ > + 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. */ > @@ -98,7 +80,6 @@ struct ad2s1210_state { > unsigned int fexcit; > bool hysteresis; > u8 resolution; > - enum ad2s1210_mode mode; > /** For reading raw sample value via SPI. */ > __be16 sample __aligned(IIO_DMA_MINALIGN); > /** SPI transmit buffer. */ > @@ -107,18 +88,15 @@ struct ad2s1210_state { > u8 tx[2]; > }; > > -static const int ad2s1210_mode_vals[4][2] = { > - [MOD_POS] = { 0, 0 }, > - [MOD_VEL] = { 0, 1 }, > - [MOD_CONFIG] = { 1, 1 }, > -}; > - > -static inline void ad2s1210_set_mode(enum ad2s1210_mode mode, > - struct ad2s1210_state *st) > +static int ad2s1210_set_mode(struct ad2s1210_state *st, enum ad2s1210_mode mode) > { > - gpiod_set_value(st->gpios[AD2S1210_A0], ad2s1210_mode_vals[mode][0]); > - gpiod_set_value(st->gpios[AD2S1210_A1], ad2s1210_mode_vals[mode][1]); > - st->mode = mode; > + struct gpio_descs *gpios = st->mode_gpios; > + DECLARE_BITMAP(bitmap, 2); > + > + bitmap[0] = mode; > + > + return gpiod_set_array_value(gpios->ndescs, gpios->desc, gpios->info, > + bitmap); > } > > /* > @@ -143,6 +121,7 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg, > .tx_buf = &st->tx[1], > }, > }; > + int ret; > > /* values can only be 7 bits, the MSB indicates an address */ > if (val & ~0x7F) > @@ -151,7 +130,9 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg, > st->tx[0] = reg; > st->tx[1] = val; > > - ad2s1210_set_mode(MOD_CONFIG, st); > + ret = ad2s1210_set_mode(st, MOD_CONFIG); > + if (ret < 0) > + return ret; > > return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers)); > } > @@ -180,7 +161,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg, > }; > int ret; > > - ad2s1210_set_mode(MOD_CONFIG, st); > + ret = ad2s1210_set_mode(st, MOD_CONFIG); > + if (ret < 0) > + return ret; > + > st->tx[0] = reg; > /* > * Must be valid register address here otherwise this could write data. > @@ -219,16 +203,16 @@ int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st) > return regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw); > } > > -static const int ad2s1210_res_pins[4][2] = { > - { 0, 0 }, {0, 1}, {1, 0}, {1, 1} > -}; > - > -static inline void ad2s1210_set_resolution_pin(struct ad2s1210_state *st) > +static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st, > + u8 resolution) > { > - gpiod_set_value(st->gpios[AD2S1210_RES0], > - ad2s1210_res_pins[(st->resolution - 10) / 2][0]); > - gpiod_set_value(st->gpios[AD2S1210_RES1], > - ad2s1210_res_pins[(st->resolution - 10) / 2][1]); > + 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 inline int ad2s1210_soft_reset(struct ad2s1210_state *st) > @@ -305,10 +289,13 @@ static ssize_t ad2s1210_store_resolution(struct device *dev, > if (ret < 0) > goto error_ret; > > - st->resolution = > - ad2s1210_resolution_value[data & AD2S1210_SET_RES]; > - ad2s1210_set_resolution_pin(st); > + ret = ad2s1210_set_resolution_gpios(st, udata); > + if (ret < 0) > + goto error_ret; > + > + st->resolution = udata; > ret = len; > + > error_ret: > mutex_unlock(&st->lock); > return ret; > @@ -339,15 +326,19 @@ static ssize_t ad2s1210_clear_fault(struct device *dev, > int ret; > > mutex_lock(&st->lock); > - gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0); > + > + gpiod_set_value(st->sample_gpio, 1); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > - gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 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->gpios[AD2S1210_SAMPLE], 0); > - gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1); > + > + gpiod_set_value(st->sample_gpio, 1); > + gpiod_set_value(st->sample_gpio, 0); > + > error_ret: > mutex_unlock(&st->lock); > > @@ -393,19 +384,19 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, > struct iio_chan_spec const *chan, > int *val) > { > - int ret = 0; > + int ret; > > mutex_lock(&st->lock); > - gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 0); > + gpiod_set_value(st->sample_gpio, 1); > /* delay (6 * tck + 20) nano seconds */ > udelay(1); > > switch (chan->type) { > case IIO_ANGL: > - ad2s1210_set_mode(MOD_POS, st); > + ret = ad2s1210_set_mode(st, MOD_POS); > break; > case IIO_ANGL_VEL: > - ad2s1210_set_mode(MOD_VEL, st); > + ret = ad2s1210_set_mode(st, MOD_VEL); > break; > default: > ret = -EINVAL; > @@ -432,7 +423,7 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, > } > > error_ret: > - gpiod_set_value(st->gpios[AD2S1210_SAMPLE], 1); > + gpiod_set_value(st->sample_gpio, 0); > /* delay (2 * tck + 20) nano seconds */ > udelay(1); > mutex_unlock(&st->lock); > @@ -546,7 +537,9 @@ static int ad2s1210_initial(struct ad2s1210_state *st) > int ret; > > mutex_lock(&st->lock); > - ad2s1210_set_resolution_pin(st); > + ret = ad2s1210_set_resolution_gpios(st, st->resolution); > + if (ret < 0) > + return ret; > > /* Use default config register value plus resolution from devicetree. */ > data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1); > @@ -612,20 +605,34 @@ static int ad2s1210_setup_clocks(struct ad2s1210_state *st) > > static int ad2s1210_setup_gpios(struct ad2s1210_state *st) > { > - struct spi_device *spi = st->sdev; > - int i, ret; > - > - for (i = 0; i < ARRAY_SIZE(gpios); i++) { > - st->gpios[i] = devm_gpiod_get(&spi->dev, gpios[i].name, > - gpios[i].flags); > - if (IS_ERR(st->gpios[i])) { > - ret = PTR_ERR(st->gpios[i]); > - dev_err(&spi->dev, > - "ad2s1210: failed to request %s GPIO: %d\n", > - gpios[i].name, ret); > - return ret; > - } > - } > + struct device *dev = &st->sdev->dev; > + > + /* should not be sampling on startup */ > + st->sample_gpio = devm_gpiod_get(dev, "sample", GPIOD_OUT_LOW); > + if (IS_ERR(st->sample_gpio)) > + return dev_err_probe(dev, PTR_ERR(st->sample_gpio), > + "failed to request sample GPIO\n"); > + > + /* both pins high means that we start in config mode */ > + st->mode_gpios = devm_gpiod_get_array(dev, "mode", GPIOD_OUT_HIGH); > + if (IS_ERR(st->mode_gpios)) > + return dev_err_probe(dev, PTR_ERR(st->mode_gpios), > + "failed to request mode GPIOs\n"); > + > + if (st->mode_gpios->ndescs != 2) > + 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), > + "failed to request resolution GPIOs\n"); > + > + if (st->resolution_gpios->ndescs != 2) > + return dev_err_probe(dev, -EINVAL, > + "requires exactly 2 resolution-gpios\n"); > > return 0; > } > @@ -690,7 +697,6 @@ static int ad2s1210_probe(struct spi_device *spi) > mutex_init(&st->lock); > st->sdev = spi; > st->hysteresis = true; > - st->mode = MOD_CONFIG; > st->resolution = 12; > st->fexcit = AD2S1210_DEF_EXCIT; > >
On Fri, 29 Sep 2023 12:53:00 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Fri, Sep 29, 2023 at 12:25 PM 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> > > --- > > ... > > > +static int ad2s1210_read_avail(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + const int **vals, int *type, > > + int *length, long mask) > > +{ > > + static const int hysteresis_available[] = { 0, 1 }; > > This is basically an enable/disable. Should the 1 value be changed to the > appropriate radians value since this is hysteresis on the position > (angle) channel? Good point. However it should be in the _raw units. The text is slightly more explicit on this for the variant of hysteresis applied to threshold events as it's added or substracted from a threshold (and thresholds are in _raw readings unless only _processed is available). Does that make 0, 1 correct as we are talking about LSB only? Jonathan > > > + > > + switch (mask) { > > + case IIO_CHAN_INFO_HYSTERESIS: > > + switch (chan->type) { > > + case IIO_ANGL: > > + *vals = hysteresis_available; > > + *type = IIO_VAL_INT; > > + *length = ARRAY_SIZE(hysteresis_available); > > + return IIO_AVAIL_LIST; > > + default: > > + return -EINVAL; > > + } > > + default: > > + return -EINVAL; > > + } > > +} > >
On Fri, 29 Sep 2023 12:23:19 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > 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> One trivial thing inline. Thanks, Jonathan > --- > > v3 changes: > * Refactored into more functions to reduce complexity of switch statements. > * Use early return instead of break in switch statements. > > drivers/staging/iio/resolver/ad2s1210.c | 86 +++++++++++++++++++++++++++++++-- > 1 file changed, 83 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 0ec3598b600a..a82cb124a12f 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -78,7 +78,6 @@ struct ad2s1210_state { > /** The external oscillator frequency in Hz. */ > unsigned long clkin_hz; > unsigned int fexcit; > - bool hysteresis; > u8 resolution; > /** For reading raw sample value via SPI. */ > __be16 sample __aligned(IIO_DMA_MINALIGN); > @@ -430,6 +429,35 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, > return ret; > } > > +static int ad2s1210_get_hysteresis(struct ad2s1210_state *st, int *val) > +{ > + int ret; > + > + mutex_lock(&st->lock); > + ret = regmap_test_bits(st->regmap, AD2S1210_REG_CONTROL, > + AD2S1210_ENABLE_HYSTERESIS); > + mutex_unlock(&st->lock); > + > + if (ret < 0) > + return ret; > + > + *val = !!ret; regmap_test_bits() is documented as returning 1 or 0 anyway. So this !! isn't necessary.. > + return IIO_VAL_INT; > +} > + > +static int ad2s1210_set_hysteresis(struct ad2s1210_state *st, int val) > +{ > + int ret; > + > + mutex_lock(&st->lock); > + ret = regmap_update_bits(st->regmap, AD2S1210_REG_CONTROL, > + AD2S1210_ENABLE_HYSTERESIS, > + val ? AD2S1210_ENABLE_HYSTERESIS : 0); > + mutex_unlock(&st->lock); > + > + return ret; > +} > + > static const int ad2s1210_velocity_scale[] = { > 17089132, /* 8.192MHz / (2*pi * 2500 / 2^15) */ > 42722830, /* 8.192MHz / (2*pi * 1000 / 2^15) */ > @@ -462,7 +490,55 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > default: > return -EINVAL; > } > + case IIO_CHAN_INFO_HYSTERESIS: > + switch (chan->type) { > + case IIO_ANGL: > + return ad2s1210_get_hysteresis(st, val); > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > + > +static int ad2s1210_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, > + int *length, long mask) > +{ > + static const int hysteresis_available[] = { 0, 1 }; > + > + switch (mask) { > + case IIO_CHAN_INFO_HYSTERESIS: > + switch (chan->type) { > + case IIO_ANGL: > + *vals = hysteresis_available; > + *type = IIO_VAL_INT; > + *length = ARRAY_SIZE(hysteresis_available); > + return IIO_AVAIL_LIST; > + default: > + return -EINVAL; > + } > + default: > + return -EINVAL; > + } > +} > > +static int ad2s1210_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int val, int val2, long mask) > +{ > + struct ad2s1210_state *st = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_HYSTERESIS: > + switch (chan->type) { > + case IIO_ANGL: > + return ad2s1210_set_hysteresis(st, val); > + default: > + return -EINVAL; > + } > default: > return -EINVAL; > } > @@ -503,7 +579,10 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > .indexed = 1, > .channel = 0, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > - BIT(IIO_CHAN_INFO_SCALE), > + BIT(IIO_CHAN_INFO_SCALE) | > + BIT(IIO_CHAN_INFO_HYSTERESIS), > + .info_mask_separate_available = > + BIT(IIO_CHAN_INFO_HYSTERESIS), > }, { > .type = IIO_ANGL_VEL, > .indexed = 1, > @@ -581,6 +660,8 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev, > > static const struct iio_info ad2s1210_info = { > .read_raw = ad2s1210_read_raw, > + .read_avail = ad2s1210_read_avail, > + .write_raw = ad2s1210_write_raw, > .attrs = &ad2s1210_attribute_group, > .debugfs_reg_access = &ad2s1210_debugfs_reg_access, > }; > @@ -696,7 +777,6 @@ static int ad2s1210_probe(struct spi_device *spi) > > mutex_init(&st->lock); > st->sdev = spi; > - st->hysteresis = true; > st->resolution = 12; > st->fexcit = AD2S1210_DEF_EXCIT; > >
On Fri, 29 Sep 2023 12:23:20 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This combines the ad2s1210_update_frequency_control_word() and > ad2s1210_soft_reset() functions into a single function since they > both have to be called together. (The software reset does not reset > any configuration registers, it only updates the excitation output > and resets the tracking loop.) > > Also clean up a few things while touching this: > - move AD2S1210_DEF_EXCIT macro with similar macros > - remove unnecessary dev_err() calls > > Signed-off-by: David Lechner <dlechner@baylibre.com> I'm sure it'll will stop me at some point but I'll keep trying to apply patches even though I skipped previous one on basis the fewer I see again, the less terrifying my inbox looks :) So applied this one and pushed out as testing. Bit of fuzz as you'd expect. Thanks, Jonathan > --- > > v3 changes: > * Expanded comment on soft reset register write. > * Fixed multiline comment style. > > drivers/staging/iio/resolver/ad2s1210.c | 66 +++++++++++++++++---------------- > 1 file changed, 34 insertions(+), 32 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index a82cb124a12f..28ab877e1bc0 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -51,12 +51,11 @@ > #define AD2S1210_MIN_CLKIN 6144000 > #define AD2S1210_MAX_CLKIN 10240000 > #define AD2S1210_MIN_EXCIT 2000 > +#define AD2S1210_DEF_EXCIT 10000 > #define AD2S1210_MAX_EXCIT 20000 > #define AD2S1210_MIN_FCW 0x4 > #define AD2S1210_MAX_FCW 0x50 > > -#define AD2S1210_DEF_EXCIT 10000 > - > enum ad2s1210_mode { > MOD_POS = 0b00, > MOD_VEL = 0b01, > @@ -188,18 +187,32 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg, > return 0; > } > > -static inline > -int ad2s1210_update_frequency_control_word(struct ad2s1210_state *st) > +/* > + * Sets the excitation frequency and performs software reset. > + * > + * Must be called with lock held. > + */ > +static int ad2s1210_reinit_excitation_frequency(struct ad2s1210_state *st, > + u16 fexcit) > { > - unsigned char fcw; > + int ret; > + u8 fcw; > > - fcw = (unsigned char)(st->fexcit * (1 << 15) / st->clkin_hz); > - if (fcw < AD2S1210_MIN_FCW || fcw > AD2S1210_MAX_FCW) { > - dev_err(&st->sdev->dev, "ad2s1210: FCW out of range\n"); > + fcw = fexcit * (1 << 15) / st->clkin_hz; > + if (fcw < AD2S1210_MIN_FCW || fcw > AD2S1210_MAX_FCW) > return -ERANGE; > - } > > - return regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw); > + ret = regmap_write(st->regmap, AD2S1210_REG_EXCIT_FREQ, fcw); > + if (ret < 0) > + return ret; > + > + st->fexcit = fexcit; > + > + /* > + * Software reset reinitializes the excitation frequency output. > + * It does not reset any of the configuration registers. > + */ > + return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0); > } > > static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st, > @@ -214,11 +227,6 @@ static int ad2s1210_set_resolution_gpios(struct ad2s1210_state *st, > bitmap); > } > > -static inline int ad2s1210_soft_reset(struct ad2s1210_state *st) > -{ > - return regmap_write(st->regmap, AD2S1210_REG_SOFT_RESET, 0); > -} > - > static ssize_t ad2s1210_show_fexcit(struct device *dev, > struct device_attribute *attr, > char *buf) > @@ -233,27 +241,24 @@ static ssize_t ad2s1210_store_fexcit(struct device *dev, > const char *buf, size_t len) > { > struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > - unsigned int fexcit; > + u16 fexcit; > int ret; > > - ret = kstrtouint(buf, 10, &fexcit); > - if (ret < 0) > - return ret; > - if (fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) { > - dev_err(dev, > - "ad2s1210: excitation frequency out of range\n"); > + ret = kstrtou16(buf, 10, &fexcit); > + if (ret < 0 || fexcit < AD2S1210_MIN_EXCIT || fexcit > AD2S1210_MAX_EXCIT) > return -EINVAL; > - } > + > mutex_lock(&st->lock); > - st->fexcit = fexcit; > - ret = ad2s1210_update_frequency_control_word(st); > + ret = ad2s1210_reinit_excitation_frequency(st, fexcit); > if (ret < 0) > goto error_ret; > - ret = ad2s1210_soft_reset(st); > + > + ret = len; > + > error_ret: > mutex_unlock(&st->lock); > > - return ret < 0 ? ret : len; > + return ret; > } > > static ssize_t ad2s1210_show_resolution(struct device *dev, > @@ -630,10 +635,8 @@ static int ad2s1210_initial(struct ad2s1210_state *st) > if (ret < 0) > goto error_ret; > > - ret = ad2s1210_update_frequency_control_word(st); > - if (ret < 0) > - goto error_ret; > - ret = ad2s1210_soft_reset(st); > + ret = ad2s1210_reinit_excitation_frequency(st, AD2S1210_DEF_EXCIT); > + > error_ret: > mutex_unlock(&st->lock); > return ret; > @@ -778,7 +781,6 @@ static int ad2s1210_probe(struct spi_device *spi) > mutex_init(&st->lock); > st->sdev = spi; > st->resolution = 12; > - st->fexcit = AD2S1210_DEF_EXCIT; > > ret = ad2s1210_setup_clocks(st); > if (ret < 0) >
On Fri, 29 Sep 2023 12:23:21 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This modifies the ad2s1210_show_fexcit() function to read the excitation > frequency from the control register. This way we don't have to keep > track of the value and don't risk returning a stale value. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Applied. A bit more fuzz with this one as context was different for the first hunk. Jonathan > --- > > v3 changes: None > > 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 28ab877e1bc0..b15d71b17266 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -76,7 +76,6 @@ struct ad2s1210_state { > struct regmap *regmap; > /** The external oscillator frequency in Hz. */ > unsigned long clkin_hz; > - unsigned int fexcit; > u8 resolution; > /** For reading raw sample value via SPI. */ > __be16 sample __aligned(IIO_DMA_MINALIGN); > @@ -206,8 +205,6 @@ static int ad2s1210_reinit_excitation_frequency(struct ad2s1210_state *st, > if (ret < 0) > return ret; > > - st->fexcit = fexcit; > - > /* > * Software reset reinitializes the excitation frequency output. > * It does not reset any of the configuration registers. > @@ -232,8 +229,22 @@ static ssize_t ad2s1210_show_fexcit(struct device *dev, > char *buf) > { > struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > + unsigned int value; > + u16 fexcit; > + int ret; > > - return sprintf(buf, "%u\n", st->fexcit); > + 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, >
On Fri, 29 Sep 2023 12:23:22 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > 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> And this is the point where things got non trivial given I skipped the hysterisis patch. This looks good to me though, so will pick it up once questions on that earlier patch are resolved. Thanks, Jonathan > --- > > v3 changes: > * This is a new patch in v3 instead of "iio: resolver: ad2s1210: rename fexcit > attribute" > > drivers/staging/iio/resolver/ad2s1210.c | 122 ++++++++++++++++++-------------- > 1 file changed, 70 insertions(+), 52 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index b15d71b17266..6accb9e3db46 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -224,54 +224,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) > @@ -474,6 +426,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) */ > @@ -506,6 +490,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: > @@ -523,9 +514,23 @@ 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, > + }; > static const int hysteresis_available[] = { 0, 1 }; > > 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: > @@ -548,6 +553,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: > @@ -560,8 +572,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, > @@ -605,11 +615,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, >
On Fri, 29 Sep 2023 12:23:23 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > Selecting the resolution was implemented as the `bits` sysfs attribute. > However, the selection of the resolution depends on how the hardware > is wired and the specific application, so this is rather a job for > devicetree to describe. > > A new devicetree property `adi,resolution` to specify the resolution > required for each chip is added and the `bits` sysfs attribute is > removed. Description need updating to reflect property having a different name. Otherwise this LGTM > > 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> ... > > +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); Doesn't match patch description of what this is called. > + if (ret < 0) > + return dev_err_probe(dev, ret, > + "failed to read assigned-resolution-bits property\n"); > + > + if (val < 10 || val > 16) > + return dev_err_probe(dev, -EINVAL, > + "resolution out of range: %u\n", val); > + > + st->resolution = (val - 10) >> 1; > + > + return 0; > +} > +
On Fri, 29 Sep 2023 12:23:24 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > The AD2S1210 chip has a phase lock range feature that allows selecting > the allowable phase difference between the excitation output and the > sine and cosine inputs. This can be set to either 44 degrees (default) > or 360 degrees. > > This patch adds a new phase channel with a threshold event that can be > used to configure the phase lock range. Actually emitting the event > will be added in a subsequent patch. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- Looks good to me.
On Fri, 29 Sep 2023 12:23:26 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > 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> > --- A style comment inline. Otherwise this an any patches before it I skipped commenting on look fine to me. > + > +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); > + A separate error path is going to be easier to read. return IIO_VAL_INT_PLUS_MICRO; error_ret: mutex_unlock(&st->lock); return ret; Of make use of the new stuff in cleanup.h etc. something like. scoped_guard(mutex)(&st->lock) { ret = regmap_read(st->regmap, ...) if (ret) return ret; ret = regmap_read(...) if (ret) return ret; } /* sysfs value is hysteresis rather than actual low value */ In general you could make could use o guard(mutex)(&st->lock); in the other functions where you hold the lock to the end of the function. Could do that as a follow on patch though and as that stuff is all very new, I'm not going to mind if you don't want to use it at all and just use the approach above. > +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; > +} >
On Fri, 29 Sep 2023 12:23:27 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > 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> I think I'm fine with treating these internal signals like this, but I would ideally like someone from Analog devices to take a look at how these are being done and make sure our interpretations of the signals make sense to them. We are pushing the boundaries a little here (though we have done similar before for fault events I think.) Jonathan
On Fri, 29 Sep 2023 12:23:28 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > The AD2S1210 has a programmable threshold for the degradation of signal > (DOS) overrange fault. This fault is triggered when either the sine or > cosine input rises the threshold voltage. 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>
On Fri, 29 Sep 2023 12:23:27 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > 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. Hmm. Looking forwards, I'm less sure that we should be shoving all these error conditions onto one channel. Fundamentally we have sine and cosine inputs. I think we should treat those as separate channels and include a third differential channel between them. So this one becomes a double event (you need to signal it on both cosine and sine channels). The DOS overange is similar. The DOS mismatch is a threshold on the differential channel giving events/in_altvoltage0_thresh_falling_value events/in_altvoltage1_thresh_falling_value (these match) events/in_altvoltage0_thresh_rising_value events/in_altvoltage1_thresh_rising_value (matches previous which is fine) events/in_altvoltage1-0_mag_rising_value Does that work here? Avoids smashing different types of signals together. We could even do the LOT as differential between two angle channels (tracking one and measured one) but meh that's getting complex. Note this will rely on channel labels to make the above make any sense at all. Jonathan > > 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> > --- > > v3 changes: This is a new patch in v3 > > drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++-- > 1 file changed, 71 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 5cc8106800d6..7abbc184c351 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, > @@ -448,6 +453,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) > { > @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, > static IIO_DEVICE_ATTR(fault, 0644, > ad2s1210_show_fault, ad2s1210_clear_fault, 0); > > -static IIO_DEVICE_ATTR(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); > @@ -745,6 +779,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, > @@ -803,12 +847,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, > @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, > __stringify(PHASE_44_DEG_TO_RAD_MICRO) " " > __stringify(PHASE_360_DEG_TO_RAD_INT) "." > __stringify(PHASE_360_DEG_TO_RAD_MICRO)); > +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); > > static struct attribute *ad2s1210_event_attributes[] = { > &iio_const_attr_in_phase0_mag_value_available.dev_attr.attr, > + &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, > NULL, > @@ -904,6 +957,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: > @@ -930,6 +990,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: >
On Sat, 30 Sep 2023 16:42:51 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 29 Sep 2023 12:23:27 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > 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. > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > error conditions onto one channel. Fundamentally we have > sine and cosine inputs. I think we should treat those as separate channels > and include a third differential channel between them. > > So this one becomes a double event (you need to signal it on both > cosine and sine channels). The DOS overange is similar. > The DOS mismatch is a threshold on the differential channel giving > > events/in_altvoltage0_thresh_falling_value > events/in_altvoltage1_thresh_falling_value (these match) > events/in_altvoltage0_thresh_rising_value > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > events/in_altvoltage1-0_mag_rising_value Sorry, got the syntax wrong :( Should have checked the ABI docs. events/in_altvoltage1-altvoltage0_mag_rising_value > > Does that work here? Avoids smashing different types of signals together. > We could even do the LOT as differential between two angle channels > (tracking one and measured one) but meh that's getting complex. > > Note this will rely on channel labels to make the above make any sense at all. > > Jonathan > > > > > > 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> > > --- > > > > v3 changes: This is a new patch in v3 > > > > drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++-- > > 1 file changed, 71 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > > index 5cc8106800d6..7abbc184c351 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, > > @@ -448,6 +453,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) > > { > > @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, > > static IIO_DEVICE_ATTR(fault, 0644, > > ad2s1210_show_fault, ad2s1210_clear_fault, 0); > > > > -static IIO_DEVICE_ATTR(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); > > @@ -745,6 +779,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, > > @@ -803,12 +847,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, > > @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, > > __stringify(PHASE_44_DEG_TO_RAD_MICRO) " " > > __stringify(PHASE_360_DEG_TO_RAD_INT) "." > > __stringify(PHASE_360_DEG_TO_RAD_MICRO)); > > +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); > > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); > > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); > > > > static struct attribute *ad2s1210_event_attributes[] = { > > &iio_const_attr_in_phase0_mag_value_available.dev_attr.attr, > > + &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, > > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, > > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, > > NULL, > > @@ -904,6 +957,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: > > @@ -930,6 +990,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: > > >
On Fri, 29 Sep 2023 12:23:30 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > The AD2S1210 has a programmable threshold for the degradation of signal > (DOS) mismatch fault. This fault is triggered when the difference in > amplitude between the sine and cosine inputs exceeds the threshold. > > The DOS reset min/max registers on the chip provide initial values > for internal tracking of the min/max of the monitor signal after the > fault register is cleared. > > This patch converts the custom device DOS reset min/max threshold > attributes custom event attributes on the monitor signal channel. > > The attributes now use millivolts instead of the raw register value in > accordance with the IIO ABI. > > Emitting the event will be implemented in a later patch. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v3 changes: This is a new patch in v3 > > .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 ++++++ > drivers/staging/iio/resolver/ad2s1210.c | 99 ++++++++++++---------- > 2 files changed, 82 insertions(+), 44 deletions(-) > > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 > new file mode 100644 > index 000000000000..ea75881b0c77 > --- /dev/null > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 > @@ -0,0 +1,27 @@ > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max Ah. So these are differential. But the mismatch channel value isn't? I also got the format wrong for differential channels. Oops. Should be the in_altvoltage0-altvoltage1 format for the previous suggestion to change that channel type to differential. This looks fine to me as new ABI. Jonathan > +KernelVersion: 6.7 > +Contact: 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-altvoltage1_thresh_rising_reset_max_available > +KernelVersion: 6.7 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns the allowable voltage range for > + in_altvoltage0-altvoltage1_thresh_rising_reset_max. > + > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min > +KernelVersion: 6.7 > +Contact: 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-altvoltage1_thresh_rising_reset_min_available > +KernelVersion: 6.7 > +Contact: linux-iio@vger.kernel.org > +Description: > + Reading returns the allowable voltage range for > + in_altvoltage0-altvoltage1_thresh_rising_reset_min. > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index aa14edbe8a77..e1c95ec73545 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -283,41 +283,6 @@ static ssize_t ad2s1210_clear_fault(struct device *dev, > return ret < 0 ? ret : len; > } > > -static ssize_t ad2s1210_show_reg(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > - unsigned int value; > - int ret; > - > - mutex_lock(&st->lock); > - ret = regmap_read(st->regmap, iattr->address, &value); > - mutex_unlock(&st->lock); > - > - return ret < 0 ? ret : sprintf(buf, "%d\n", value); > -} > - > -static ssize_t ad2s1210_store_reg(struct device *dev, > - struct device_attribute *attr, > - const char *buf, size_t len) > -{ > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > - unsigned char data; > - int ret; > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > - > - ret = kstrtou8(buf, 10, &data); > - if (ret) > - return -EINVAL; > - > - mutex_lock(&st->lock); > - ret = regmap_write(st->regmap, iattr->address, data); > - mutex_unlock(&st->lock); > - return ret < 0 ? ret : len; > -} > - > static int ad2s1210_single_conversion(struct ad2s1210_state *st, > struct iio_chan_spec const *chan, > int *val) > @@ -743,13 +708,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, > static IIO_DEVICE_ATTR(fault, 0644, > ad2s1210_show_fault, ad2s1210_clear_fault, 0); > > -static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644, > - ad2s1210_show_reg, ad2s1210_store_reg, > - AD2S1210_REG_DOS_RST_MAX_THRD); > -static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644, > - ad2s1210_show_reg, ad2s1210_store_reg, > - AD2S1210_REG_DOS_RST_MIN_THRD); > - > static const struct iio_event_spec ad2s1210_position_event_spec[] = { > { > /* Tracking error exceeds LOT threshold fault. */ > @@ -867,8 +825,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > > static struct attribute *ad2s1210_attributes[] = { > &iio_dev_attr_fault.dev_attr.attr, > - &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr, > - &iio_dev_attr_dos_rst_min_thrd.dev_attr.attr, > NULL, > }; > > @@ -876,6 +832,49 @@ static const struct attribute_group ad2s1210_attribute_group = { > .attrs = ad2s1210_attributes, > }; > > +static ssize_t event_attr_voltage_reg_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > + unsigned int value; > + int ret; > + > + mutex_lock(&st->lock); > + ret = regmap_read(st->regmap, iattr->address, &value); > + mutex_unlock(&st->lock); > + > + if (ret < 0) > + return ret; > + > + return sprintf(buf, "%d\n", value * THRESHOLD_MILLIVOLT_PER_LSB); > +} > + > +static ssize_t event_attr_voltage_reg_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t len) > +{ > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > + u16 data; > + int ret; > + > + ret = kstrtou16(buf, 10, &data); > + if (ret) > + return -EINVAL; > + > + mutex_lock(&st->lock); > + ret = regmap_write(st->regmap, iattr->address, > + data / THRESHOLD_MILLIVOLT_PER_LSB); > + mutex_unlock(&st->lock); > + > + if (ret < 0) > + return ret; > + > + return len; > +} > + > static ssize_t > in_angl1_thresh_rising_value_available_show(struct device *dev, > struct device_attribute *attr, > @@ -906,6 +905,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, > IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); > IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR); > IIO_CONST_ATTR(in_altvoltage0_mag_value_available, THRESHOLD_RANGE_STR); > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_max, 0644, > + event_attr_voltage_reg_show, event_attr_voltage_reg_store, > + AD2S1210_REG_DOS_RST_MAX_THRD); > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_max_available, THRESHOLD_RANGE_STR); > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_min, 0644, > + event_attr_voltage_reg_show, event_attr_voltage_reg_store, > + AD2S1210_REG_DOS_RST_MIN_THRD); > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_min_available, THRESHOLD_RANGE_STR); > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); > > @@ -914,6 +921,10 @@ static struct attribute *ad2s1210_event_attributes[] = { > &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, > &iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr, > &iio_const_attr_in_altvoltage0_mag_value_available.dev_attr.attr, > + &iio_dev_attr_in_altvoltage0_mag_reset_max.dev_attr.attr, > + &iio_const_attr_in_altvoltage0_mag_reset_max_available.dev_attr.attr, > + &iio_dev_attr_in_altvoltage0_mag_reset_min.dev_attr.attr, > + &iio_const_attr_in_altvoltage0_mag_reset_min_available.dev_attr.attr, > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, > NULL, >
On Fri, 29 Sep 2023 12:23:31 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > When reading the position and velocity on the AD2S1210, there is also a > 3rd byte following the two data bytes that contains the fault flag bits. > This patch adds support for reading this byte and generating events when > faults occur. > > The faults are mapped to various channels and event types in order to > have a unique event for each fault. > > Signed-off-by: David Lechner <dlechner@baylibre.com> Use of x and y modifiers is a little odd. What was your reasoning? Was it just that there was a X_OR_Y modifier? If so, don't use that! It seemed like a good idea at the time, but it's not nice to deal with and requires a channel with that modifier to hang the controls off + make sure userspace expects that event code. > --- > > v3 changes: This is a new patch in v3 > > drivers/staging/iio/resolver/ad2s1210.c | 175 +++++++++++++++++++++++++++++--- > 1 file changed, 161 insertions(+), 14 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index e1c95ec73545..dc3cc3ab855e 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -21,6 +21,7 @@ > #include <linux/types.h> > > #include <linux/iio/buffer.h> > +#include <linux/iio/events.h> > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > #include <linux/iio/trigger_consumer.h> > @@ -35,6 +36,16 @@ > #define AD2S1210_SET_ENRES GENMASK(3, 2) > #define AD2S1210_SET_RES GENMASK(1, 0) > > +/* fault register flags */ > +#define AD2S1210_FAULT_CLIP BIT(7) > +#define AD2S1210_FAULT_LOS BIT(6) > +#define AD2S1210_FAULT_DOS_OVR BIT(5) > +#define AD2S1210_FAULT_DOS_MIS BIT(4) > +#define AD2S1210_FAULT_LOT BIT(3) > +#define AD2S1210_FAULT_VELOCITY BIT(2) > +#define AD2S1210_FAULT_PHASE BIT(1) > +#define AD2S1210_FAULT_CONFIG_PARITY BIT(0) > + > #define AD2S1210_REG_POSITION_MSB 0x80 > #define AD2S1210_REG_POSITION_LSB 0x81 > #define AD2S1210_REG_VELOCITY_MSB 0x82 > @@ -71,6 +82,8 @@ > /* max voltage for threshold registers is 0x7F * 38 mV */ > #define THRESHOLD_RANGE_STR "[0 38 4826]" > > +#define FAULT_ONESHOT(bit, new, old) (new & bit && !(old & bit)) > + > enum ad2s1210_mode { > MOD_POS = 0b00, > MOD_VEL = 0b01, > @@ -98,8 +111,13 @@ struct ad2s1210_state { > unsigned long clkin_hz; > /** The selected resolution */ > enum ad2s1210_resolution resolution; > + /** Copy of fault register from the previous read. */ > + u8 prev_fault_flags; > /** For reading raw sample value via SPI. */ > - __be16 sample __aligned(IIO_DMA_MINALIGN); > + struct { > + __be16 raw; > + u8 fault; > + } sample __aligned(IIO_DMA_MINALIGN);; > /** Scan buffer */ > struct { > __be16 chan[2]; > @@ -158,7 +176,15 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg, > if (ret < 0) > return ret; > > - return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers)); > + ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers)); > + if (ret < 0) > + return ret; > + > + /* soft reset also clears the fault register */ > + if (reg == AD2S1210_REG_SOFT_RESET) > + st->prev_fault_flags = 0; > + > + return 0; > } > > /* > @@ -200,6 +226,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg, > if (ret < 0) > return ret; > > + /* reading the fault register also clears it */ > + if (reg == AD2S1210_REG_FAULT) > + st->prev_fault_flags = 0; > + > /* > * If the D7 bit is set on any read/write register, it indicates a > * parity error. The fault register is read-only and the D7 bit means > @@ -283,14 +313,92 @@ static ssize_t ad2s1210_clear_fault(struct device *dev, > return ret < 0 ? ret : len; > } > > -static int ad2s1210_single_conversion(struct ad2s1210_state *st, > +static void ad2s1210_push_events(struct iio_dev *indio_dev, > + u8 flags, s64 timestamp) > +{ > + struct ad2s1210_state *st = iio_priv(indio_dev); > + > + /* Sine/cosine inputs clipped */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_CLIP, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_MOD_EVENT_CODE(IIO_ALTVOLTAGE, 1, > + IIO_MOD_X_OR_Y, Hmm. So this is a weird corner I'd forgotten about and explains your use of X and Y modifiers. Long ago this was added to support a similar case to the one you have, but mixing and matching modifiers like this doesn't scale, so I think we'd be better just signally events on both channels. If nothing else, someone waiting for an event on a specific channel isn't looking for this weird modifier. When it was used before we added a channel for MOD_X_OR_Y so events were enabled on that. It's horrible though so don't do that. > + IIO_EV_TYPE_MAG, > + IIO_EV_DIR_NONE), > + timestamp); > + > + /* Sine/cosine inputs below LOS threshold */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_LOS, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_FALLING), > + timestamp); > + > + /* Sine/cosine inputs exceed DOS overrange threshold */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_OVR, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_RISING), > + timestamp); > + > + /* Sine/cosine inputs exceed DOS mismatch threshold */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_MIS, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0, > + IIO_EV_TYPE_MAG, > + IIO_EV_DIR_NONE), > + timestamp); > + > + /* Tracking error exceeds LOT threshold */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_LOT, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_ANGL, 1, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_RISING), > + timestamp); > + > + /* Velocity exceeds maximum tracking rate */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_VELOCITY, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_ANGL_VEL, 0, > + IIO_EV_TYPE_THRESH, > + IIO_EV_DIR_RISING), > + timestamp); > + > + /* Phase error exceeds phase lock range */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_PHASE, flags, st->prev_fault_flags)) > + iio_push_event(indio_dev, > + IIO_UNMOD_EVENT_CODE(IIO_PHASE, 0, > + IIO_EV_TYPE_MAG, > + IIO_EV_DIR_NONE), > + timestamp); > + > + /* Configuration parity error */ > + if (FAULT_ONESHOT(AD2S1210_FAULT_CONFIG_PARITY, flags, > + st->prev_fault_flags)) > + /* > + * Userspace should also get notified of this via error return > + * when trying to write to any attribute that writes a register. > + */ > + dev_err_ratelimited(&indio_dev->dev, > + "Configuration parity error\n"); > + > + st->prev_fault_flags = flags; > +} > + > +static int ad2s1210_single_conversion(struct iio_dev *indio_dev, > struct iio_chan_spec const *chan, > int *val) > { > + struct ad2s1210_state *st = iio_priv(indio_dev); > + s64 timestamp; > int ret; > > mutex_lock(&st->lock); > gpiod_set_value(st->sample_gpio, 1); > + timestamp = iio_get_time_ns(indio_dev); > /* delay (6 * tck + 20) nano seconds */ > udelay(1); > > @@ -307,17 +415,17 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, > } > if (ret < 0) > goto error_ret; > - ret = spi_read(st->sdev, &st->sample, 2); > + ret = spi_read(st->sdev, &st->sample, 3); > if (ret < 0) > goto error_ret; > > switch (chan->type) { > case IIO_ANGL: > - *val = be16_to_cpu(st->sample); > + *val = be16_to_cpu(st->sample.raw); > ret = IIO_VAL_INT; > break; > case IIO_ANGL_VEL: > - *val = (s16)be16_to_cpu(st->sample); > + *val = (s16)be16_to_cpu(st->sample.raw); > ret = IIO_VAL_INT; > break; > default: > @@ -325,6 +433,8 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, > break; > } > > + ad2s1210_push_events(indio_dev, st->rx[2], timestamp); > + > error_ret: > gpiod_set_value(st->sample_gpio, 0); > /* delay (2 * tck + 20) nano seconds */ > @@ -608,7 +718,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, > > switch (mask) { > case IIO_CHAN_INFO_RAW: > - return ad2s1210_single_conversion(st, chan, val); > + return ad2s1210_single_conversion(indio_dev, chan, val); > case IIO_CHAN_INFO_SCALE: > switch (chan->type) { > case IIO_ANGL: > @@ -721,6 +831,14 @@ static const struct iio_event_spec ad2s1210_position_event_spec[] = { > }, > }; > > +static const struct iio_event_spec ad2s1210_velocity_event_spec[] = { > + { > + /* Velocity exceeds maximum tracking rate fault. */ > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_RISING, > + }, > +}; > + > static const struct iio_event_spec ad2s1210_phase_event_spec[] = { > { > /* Phase error fault. */ > @@ -754,6 +872,14 @@ static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = { > }, > }; > > +static const struct iio_event_spec ad2s1210_sin_cos_event_spec[] = { > + { > + /* Sine/cosine clipping fault. */ > + .type = IIO_EV_TYPE_MAG, > + .dir = IIO_EV_DIR_NONE, > + }, > +}; > + > static const struct iio_chan_spec ad2s1210_channels[] = { > { > .type = IIO_ANGL, > @@ -784,6 +910,8 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > }, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > BIT(IIO_CHAN_INFO_SCALE), > + .event_spec = ad2s1210_velocity_event_spec, > + .num_event_specs = ARRAY_SIZE(ad2s1210_velocity_event_spec), > }, > IIO_CHAN_SOFT_TIMESTAMP(2), > { > @@ -820,6 +948,26 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > .scan_index = -1, > .event_spec = ad2s1210_monitor_signal_event_spec, > .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec), > + }, { > + /* sine input */ > + .type = IIO_ALTVOLTAGE, > + .indexed = 1, > + .channel = 1, > + .modified = 1, > + .channel2 = IIO_MOD_Y, A bit odd and I'm not sure it's beneficial over just using an index and a label for the channel. If a modifier is necessary then we could add a new one for this. Another option might be to provide in_altvoltage0_phase or... can we map this to i (inphase) and q (quadrature) phases? Sort of feels like we can, and those modifiers already exist. Note though that if we do that, we'll need to modify the various ABI docs etc to include the modifiers whenever it's about the sine and cosine channels. > + .scan_index = -1, > + .event_spec = ad2s1210_sin_cos_event_spec, > + .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec), > + }, { > + /* cosine input */ > + .type = IIO_ALTVOLTAGE, > + .indexed = 1, > + .channel = 1, > + .modified = 1, > + .channel2 = IIO_MOD_X, > + .scan_index = -1, > + .event_spec = ad2s1210_sin_cos_event_spec, > + .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec), > }, > }; > > @@ -936,7 +1084,7 @@ static const struct attribute_group ad2s1210_event_attribute_group = { > > static int ad2s1210_initial(struct ad2s1210_state *st) > { > - unsigned char data; > + unsigned int data; > int ret; > > mutex_lock(&st->lock); > @@ -1073,12 +1221,11 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p) > if (ret < 0) > goto error_ret; > > - /* REVIST: we can read 3 bytes here and also get fault flags */ > - ret = spi_read(st->sdev, st->rx, 2); > + ret = spi_read(st->sdev, &st->sample, 3); > if (ret < 0) > goto error_ret; > > - memcpy(&st->scan.chan[chan++], st->rx, 2); > + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2); > } > > if (test_bit(1, indio_dev->active_scan_mask)) { > @@ -1086,14 +1233,14 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p) > if (ret < 0) > goto error_ret; > > - /* REVIST: we can read 3 bytes here and also get fault flags */ > - ret = spi_read(st->sdev, st->rx, 2); > + ret = spi_read(st->sdev, &st->sample, 3); > if (ret < 0) > goto error_ret; > > - memcpy(&st->scan.chan[chan++], st->rx, 2); > + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2); > } > > + ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp); > iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp); > > error_ret: >
On Sat, Sep 30, 2023 at 10:00 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Sep 2023 12:53:00 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Fri, Sep 29, 2023 at 12:25 PM 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> > > > --- > > > > ... > > > > > +static int ad2s1210_read_avail(struct iio_dev *indio_dev, > > > + struct iio_chan_spec const *chan, > > > + const int **vals, int *type, > > > + int *length, long mask) > > > +{ > > > + static const int hysteresis_available[] = { 0, 1 }; > > > > This is basically an enable/disable. Should the 1 value be changed to the > > appropriate radians value since this is hysteresis on the position > > (angle) channel? > > Good point. However it should be in the _raw units. The text is > slightly more explicit on this for > the variant of hysteresis applied to threshold events as it's > added or substracted from a threshold (and thresholds are in > _raw readings unless only _processed is available). > > Does that make 0, 1 correct as we are talking about LSB only? > It is currently only correct (as a raw value) if the selected resolution is 16-bit. So I will need to fix this so the value is `1 << (16 - resolution)`.
On Fri, Sep 29, 2023 at 12:23:07PM -0500, David Lechner wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > This fixes a use before initialization in ad2s1210_probe(). The > ad2s1210_setup_gpios() function uses st->sdev but it was being called > before this field was initialized. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > Fixes: b19e9ad5e2cb ("staging:iio:resolver:ad2s1210 general driver cleanup.") This would crash the driver right away, on probe. It's amazing no one filed a bug report even though the bug is 12 years old. regards, dan carpenter
On Mon, 2 Oct 2023 11:07:15 +0300 Dan Carpenter <dan.carpenter@linaro.org> wrote: > On Fri, Sep 29, 2023 at 12:23:07PM -0500, David Lechner wrote: > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > This fixes a use before initialization in ad2s1210_probe(). The > > ad2s1210_setup_gpios() function uses st->sdev but it was being called > > before this field was initialized. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > > > Fixes: b19e9ad5e2cb ("staging:iio:resolver:ad2s1210 general driver cleanup.") Thanks but nope, not that one. At that point ad2s1210_setup_gpios, didn't use st->sdev. I think this went wrong when the platform data was removed in I 'think' it was Fixes: f356dc6ec26b ("staging: iio: ad2s1210: Switch to the gpio descriptor interface") > > This would crash the driver right away, on probe. It's amazing no one > filed a bug report even though the bug is 12 years old. Only 5 years :) Welcome to the long tail of IIO Devices and the long term availability of the hardware - this is still a production part. Clearly no one was using the upstream driver for 5 + years, but here comes David who is not only fixing the bugs but cleaning it up. Hmm. What happened to roadtest? I was hoping that would solve this sort of issue by allowing simple testing of basic functionality... Hope it is still headed for a new version / upstream! Jonathan > regards, > dan carpenter
On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Sep 2023 12:23:27 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > 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. > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > error conditions onto one channel. Fundamentally we have > sine and cosine inputs. I think we should treat those as separate channels > and include a third differential channel between them. At first, I did consider a differential channel as you suggested in v2. However, the datasheet is quite clear that the LOS and DOS faults (and only those faults) come from a signal it calls the "monitor signal". This signal is defined as: Monitor = A1 * sin(theta) * sin(phi) + A2 * cos(theta) * cos(phi) where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the cosine input and phi is the position output. So mathematically speaking, there is no signal that is the difference between the two inputs. (See "Theory of Operation" section in the datasheet.) But if we want to hide these internal details and don't care about a strict definition of "differential", then what is suggested below seems fine. > > So this one becomes a double event (you need to signal it on both > cosine and sine channels). The DOS overange is similar. > The DOS mismatch is a threshold on the differential channel giving > > events/in_altvoltage0_thresh_falling_value > events/in_altvoltage1_thresh_falling_value (these match) > events/in_altvoltage0_thresh_rising_value > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > events/in_altvoltage1-altvoltage0_mag_rising_value > > Does that work here? Avoids smashing different types of signals together. > We could even do the LOT as differential between two angle channels > (tracking one and measured one) but meh that's getting complex.> > Note this will rely on channel labels to make the above make any sense at all. I think this could be OK - I think what matters most is having some documentation that maps the faults and registers on the chip to the iio names. Where would the sine/cosine clipping fault fit in though? I got a bit too creative and used X_OR_Y to differentiate it (see discussion in "staging: iio: resolver: ad2s1210: implement fault events"). Strictly speaking, it should probably be a type: threshold, direction: either event on both the sine and cosine input channels (another double event) since it occurs if either of the signal exceeds the power or ground rail voltage. But we already have threshold rising and threshold falling on these channels with a different meaning. I guess it could call it magnitude instead of a threshold?
On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Sep 2023 12:23:31 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > When reading the position and velocity on the AD2S1210, there is also a > > 3rd byte following the two data bytes that contains the fault flag bits. > > This patch adds support for reading this byte and generating events when > > faults occur. > > > > The faults are mapped to various channels and event types in order to > > have a unique event for each fault. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > Use of x and y modifiers is a little odd. What was your reasoning? > Was it just that there was a X_OR_Y modifier? If so, don't use that! > It seemed like a good idea at the time, but it's not nice to deal with > and requires a channel with that modifier to hang the controls off > + make sure userspace expects that event code. > Yes, I was perhaps getting a bit too creative here (the two inputs come from transformers mounted 90 degrees apart so measure X and Y component of an angle). The only reason I did this was to take advantage of the possibility of an "OR" event to avoid double events for a single fault bit. We can just go with the double event on the two different input channels as discussed in "[PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr".
On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Sep 2023 12:23:31 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > When reading the position and velocity on the AD2S1210, there is also a > > 3rd byte following the two data bytes that contains the fault flag bits. > > This patch adds support for reading this byte and generating events when > > faults occur. > > > > The faults are mapped to various channels and event types in order to > > have a unique event for each fault. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > Use of x and y modifiers is a little odd. What was your reasoning? > Was it just that there was a X_OR_Y modifier? If so, don't use that! > It seemed like a good idea at the time, but it's not nice to deal with > and requires a channel with that modifier to hang the controls off > + make sure userspace expects that event code. Regarding the point about "requires a channel with that modifier to hang the controls off...". Although that comment was about modifiers, does it also apply in general. There are several fault events that don't have any configurable parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds max tracking rate_. So there won't be any attributes that contain the event specification for those (e.g. no `events/in_angl0_*` attributes). It sounds like this would be a problem as well? Should we consider a IIO_EV_INFO_LABEL so that we can have some sort of attribute (namely `events/<dir>_<channel spec>_label`) so that userspace can enumerate expected events for non-configurable events?
On Sat, Sep 30, 2023 at 10:47 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Sep 2023 12:23:30 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > The AD2S1210 has a programmable threshold for the degradation of signal > > (DOS) mismatch fault. This fault is triggered when the difference in > > amplitude between the sine and cosine inputs exceeds the threshold. > > > > The DOS reset min/max registers on the chip provide initial values > > for internal tracking of the min/max of the monitor signal after the > > fault register is cleared. > > > > This patch converts the custom device DOS reset min/max threshold > > attributes custom event attributes on the monitor signal channel. > > > > The attributes now use millivolts instead of the raw register value in > > accordance with the IIO ABI. > > > > Emitting the event will be implemented in a later patch. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > > > v3 changes: This is a new patch in v3 > > > > .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 ++++++ > > drivers/staging/iio/resolver/ad2s1210.c | 99 ++++++++++++---------- > > 2 files changed, 82 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 > > new file mode 100644 > > index 000000000000..ea75881b0c77 > > --- /dev/null > > +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-resolver-ad2s1210 > > @@ -0,0 +1,27 @@ > > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_max > Ah. So these are differential. But the mismatch channel value isn't? > > I also got the format wrong for differential channels. Oops. Should > be the in_altvoltage0-altvoltage1 format for the previous suggestion > to change that channel type to differential. > > This looks fine to me as new ABI. > > Jonathan > As discussed in <https://lore.kernel.org/linux-iio/CAMknhBFKSqXvgOeRjGAOfURzndmxmCffdU6MUirEmfzKqwM_Kg@mail.gmail.com/>, technically no they are not differential. I started to implement it that way but changed my mind after understanding the datasheet more in depth. It looks like I forgot to update the documentation in this patch to match the final implementation that was submitted. In any case, I will update the documentation to match the implementation or vice versa depending on what we decide on for which channel the events should be associated with. > > > > +KernelVersion: 6.7 > > +Contact: 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-altvoltage1_thresh_rising_reset_max_available > > +KernelVersion: 6.7 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Reading returns the allowable voltage range for > > + in_altvoltage0-altvoltage1_thresh_rising_reset_max. > > + > > +What: /sys/bus/iio/devices/iio:deviceX/events/in_altvoltage0-altvoltage1_thresh_rising_reset_min > > +KernelVersion: 6.7 > > +Contact: 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-altvoltage1_thresh_rising_reset_min_available > > +KernelVersion: 6.7 > > +Contact: linux-iio@vger.kernel.org > > +Description: > > + Reading returns the allowable voltage range for > > + in_altvoltage0-altvoltage1_thresh_rising_reset_min. > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > > index aa14edbe8a77..e1c95ec73545 100644 > > --- a/drivers/staging/iio/resolver/ad2s1210.c > > +++ b/drivers/staging/iio/resolver/ad2s1210.c > > @@ -283,41 +283,6 @@ static ssize_t ad2s1210_clear_fault(struct device *dev, > > return ret < 0 ? ret : len; > > } > > > > -static ssize_t ad2s1210_show_reg(struct device *dev, > > - struct device_attribute *attr, > > - char *buf) > > -{ > > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > > - unsigned int value; > > - int ret; > > - > > - mutex_lock(&st->lock); > > - ret = regmap_read(st->regmap, iattr->address, &value); > > - mutex_unlock(&st->lock); > > - > > - return ret < 0 ? ret : sprintf(buf, "%d\n", value); > > -} > > - > > -static ssize_t ad2s1210_store_reg(struct device *dev, > > - struct device_attribute *attr, > > - const char *buf, size_t len) > > -{ > > - struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > > - unsigned char data; > > - int ret; > > - struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > > - > > - ret = kstrtou8(buf, 10, &data); > > - if (ret) > > - return -EINVAL; > > - > > - mutex_lock(&st->lock); > > - ret = regmap_write(st->regmap, iattr->address, data); > > - mutex_unlock(&st->lock); > > - return ret < 0 ? ret : len; > > -} > > - > > static int ad2s1210_single_conversion(struct ad2s1210_state *st, > > struct iio_chan_spec const *chan, > > int *val) > > @@ -743,13 +708,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, > > static IIO_DEVICE_ATTR(fault, 0644, > > ad2s1210_show_fault, ad2s1210_clear_fault, 0); > > > > -static IIO_DEVICE_ATTR(dos_rst_max_thrd, 0644, > > - ad2s1210_show_reg, ad2s1210_store_reg, > > - AD2S1210_REG_DOS_RST_MAX_THRD); > > -static IIO_DEVICE_ATTR(dos_rst_min_thrd, 0644, > > - ad2s1210_show_reg, ad2s1210_store_reg, > > - AD2S1210_REG_DOS_RST_MIN_THRD); > > - > > static const struct iio_event_spec ad2s1210_position_event_spec[] = { > > { > > /* Tracking error exceeds LOT threshold fault. */ > > @@ -867,8 +825,6 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > > > > static struct attribute *ad2s1210_attributes[] = { > > &iio_dev_attr_fault.dev_attr.attr, > > - &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr, > > - &iio_dev_attr_dos_rst_min_thrd.dev_attr.attr, > > NULL, > > }; > > > > @@ -876,6 +832,49 @@ static const struct attribute_group ad2s1210_attribute_group = { > > .attrs = ad2s1210_attributes, > > }; > > > > +static ssize_t event_attr_voltage_reg_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > > + unsigned int value; > > + int ret; > > + > > + mutex_lock(&st->lock); > > + ret = regmap_read(st->regmap, iattr->address, &value); > > + mutex_unlock(&st->lock); > > + > > + if (ret < 0) > > + return ret; > > + > > + return sprintf(buf, "%d\n", value * THRESHOLD_MILLIVOLT_PER_LSB); > > +} > > + > > +static ssize_t event_attr_voltage_reg_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t len) > > +{ > > + struct ad2s1210_state *st = iio_priv(dev_to_iio_dev(dev)); > > + struct iio_dev_attr *iattr = to_iio_dev_attr(attr); > > + u16 data; > > + int ret; > > + > > + ret = kstrtou16(buf, 10, &data); > > + if (ret) > > + return -EINVAL; > > + > > + mutex_lock(&st->lock); > > + ret = regmap_write(st->regmap, iattr->address, > > + data / THRESHOLD_MILLIVOLT_PER_LSB); > > + mutex_unlock(&st->lock); > > + > > + if (ret < 0) > > + return ret; > > + > > + return len; > > +} > > + > > static ssize_t > > in_angl1_thresh_rising_value_available_show(struct device *dev, > > struct device_attribute *attr, > > @@ -906,6 +905,14 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, > > IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); > > IIO_CONST_ATTR(in_altvoltage0_thresh_rising_value_available, THRESHOLD_RANGE_STR); > > IIO_CONST_ATTR(in_altvoltage0_mag_value_available, THRESHOLD_RANGE_STR); > > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_max, 0644, > > + event_attr_voltage_reg_show, event_attr_voltage_reg_store, > > + AD2S1210_REG_DOS_RST_MAX_THRD); > > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_max_available, THRESHOLD_RANGE_STR); > > +IIO_DEVICE_ATTR(in_altvoltage0_mag_reset_min, 0644, > > + event_attr_voltage_reg_show, event_attr_voltage_reg_store, > > + AD2S1210_REG_DOS_RST_MIN_THRD); > > +IIO_CONST_ATTR(in_altvoltage0_mag_reset_min_available, THRESHOLD_RANGE_STR); > > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); > > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); These attribute names don't match the documentation above. :-/ > > > > @@ -914,6 +921,10 @@ static struct attribute *ad2s1210_event_attributes[] = { > > &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, > > &iio_const_attr_in_altvoltage0_thresh_rising_value_available.dev_attr.attr, > > &iio_const_attr_in_altvoltage0_mag_value_available.dev_attr.attr, > > + &iio_dev_attr_in_altvoltage0_mag_reset_max.dev_attr.attr, > > + &iio_const_attr_in_altvoltage0_mag_reset_max_available.dev_attr.attr, > > + &iio_dev_attr_in_altvoltage0_mag_reset_min.dev_attr.attr, > > + &iio_const_attr_in_altvoltage0_mag_reset_min_available.dev_attr.attr, > > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, > > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, > > NULL, > > >
On Mon, Oct 2, 2023 at 11:58 AM David Lechner <dlechner@baylibre.com> wrote: > > On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 29 Sep 2023 12:23:31 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > From: David Lechner <david@lechnology.com> > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > When reading the position and velocity on the AD2S1210, there is also a > > > 3rd byte following the two data bytes that contains the fault flag bits. > > > This patch adds support for reading this byte and generating events when > > > faults occur. > > > > > > The faults are mapped to various channels and event types in order to > > > have a unique event for each fault. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > Use of x and y modifiers is a little odd. What was your reasoning? > > Was it just that there was a X_OR_Y modifier? If so, don't use that! > > It seemed like a good idea at the time, but it's not nice to deal with > > and requires a channel with that modifier to hang the controls off > > + make sure userspace expects that event code. > > > Regarding the point about "requires a channel with that modifier to > hang the controls off...". Although that comment was about modifiers, > does it also apply in general. > > There are several fault events that don't have any configurable > parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds > max tracking rate_. So there won't be any attributes that contain the > event specification for those (e.g. no `events/in_angl0_*` > attributes). It sounds like this would be a problem as well? > > Should we consider a IIO_EV_INFO_LABEL so that we can have some sort > of attribute (namely `events/<dir>_<channel spec>_label`) so that > userspace can enumerate expected events for non-configurable events? Well, I didn't think that all the way through before I hit send. IIO_EV_INFO_LABEL is clearly not the right way to implement a `events/*_label` attribute, but however we consider going about it, the point of adding such an attribute was the main idea.
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Samstag, 30. September 2023 17:32 > To: David Lechner <dlechner@baylibre.com> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > staging@lists.linux.dev; David Lechner <david@lechnology.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno > <Nuno.Sa@analog.com>; Axel Haslam <ahaslam@baylibre.com>; Philip Molloy > <pmolloy@baylibre.com>; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS > threshold to event attr > > > On Fri, 29 Sep 2023 12:23:27 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > 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> > > I think I'm fine with treating these internal signals like this, but I would ideally > like someone from Analog devices to take a look at how these are being done > and make sure our interpretations of the signals make sense to them. We are > pushing the boundaries a little here (though we have done similar before for > fault events I think.) Hi Jonathan, David and I we also had some internal discussion related to this. I'm sure these fault events and thresholds are understood correctly. Doing it this or the other way, it needs to be properly documented in order to make sense. So from my perspective whatever makes the most sense from a IIO ABI perspective, is the way to forward. -Michael > > Jonathan
On Sat, 30 Sep 2023 15:55:36 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 29 Sep 2023 12:23:18 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > - Remove "adi," prefix from gpio names. > > - Sample gpio is now expected to be active low. > > - Convert A0 and A1 gpios to "mode-gpios" gpio array. > > - Convert RES0 and RES1 gpios to "resolution-gpios" gpio array. > > - Remove extraneous lookup tables. > > - Remove unused mode field from state struct. > > - Swap argument order of ad2s1210_set_mode() while we are touching this. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > Applied, 0-day ran smatch on this and it picked up that a log isn't released in an error path. I've fixed that up with a goto error_ret and will push out a fresh testing branch for 0-day to take another look at. ... > > @@ -546,7 +537,9 @@ static int ad2s1210_initial(struct ad2s1210_state *st) > > int ret; > > > > mutex_lock(&st->lock); > > - ad2s1210_set_resolution_pin(st); > > + ret = ad2s1210_set_resolution_gpios(st, st->resolution); > > + if (ret < 0) Exiting with lock held. There is an error_ret label that releases the lock so use that. > > + return ret; > > > > /* Use default config register value plus resolution from devicetree. */ > > data = FIELD_PREP(AD2S1210_PHASE_LOCK_RANGE_44, 1);
On Wed, 4 Oct 2023 11:01:56 +0000 "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Samstag, 30. September 2023 17:32 > > To: David Lechner <dlechner@baylibre.com> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > > staging@lists.linux.dev; David Lechner <david@lechnology.com>; Rob Herring > > <robh+dt@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno > > <Nuno.Sa@analog.com>; Axel Haslam <ahaslam@baylibre.com>; Philip Molloy > > <pmolloy@baylibre.com>; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS > > threshold to event attr > > > > > > On Fri, 29 Sep 2023 12:23:27 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > From: David Lechner <david@lechnology.com> > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > 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> > > > > I think I'm fine with treating these internal signals like this, but I would ideally > > like someone from Analog devices to take a look at how these are being done > > and make sure our interpretations of the signals make sense to them. We are > > pushing the boundaries a little here (though we have done similar before for > > fault events I think.) > > Hi Jonathan, > David and I we also had some internal discussion related to this. > I'm sure these fault events and thresholds are understood correctly. > Doing it this or the other way, it needs to be properly documented in order to make sense. > So from my perspective whatever makes the most sense from a IIO ABI > perspective, is the way to forward. Great - as long as keep to a logical mapping I quite like the events approach. Most of these faults are real thresholds on things being measured (even if those 'things' are signals from which stuff is derived for the main measurements the device is making.) Jonathan > > -Michael > > > > > Jonathan >
On Mon, 2 Oct 2023 11:09:11 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 29 Sep 2023 12:23:27 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > From: David Lechner <david@lechnology.com> > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > 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. > > > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > > error conditions onto one channel. Fundamentally we have > > sine and cosine inputs. I think we should treat those as separate channels > > and include a third differential channel between them. > > At first, I did consider a differential channel as you suggested in > v2. However, the datasheet is quite clear that the LOS and DOS faults > (and only those faults) come from a signal it calls the "monitor > signal". This signal is defined as: > > Monitor = A1 * sin(theta) * sin(phi) + A2 * cos(theta) * cos(phi) > > where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the > cosine input and phi is the position output. So mathematically > speaking, there is no signal that is the difference between the two > inputs. (See "Theory of Operation" section in the datasheet.) Hmm. That's certainly a bit more complex than I expected. Relying on the brief description led me astray. It's related to the differences in the measured and as if theta == phi and A1 == A2 (ideal) then it will be A1. I can see it's relevant to DOS, but not LOS. The description of LOS seems to overlap a number of different things unfortunately. > > But if we want to hide these internal details and don't care about a > strict definition of "differential", then what is suggested below > seems fine. Probably best to introduce that monitor signal though we'll have to be a bit vague about what it is which has the side effect that anyone trying to understand what on earth these faults are is going to be confused (having read the datasheet section a couple of times I'm not 100% sure...) > > > > > So this one becomes a double event (you need to signal it on both > > cosine and sine channels). The DOS overange is similar. > > The DOS mismatch is a threshold on the differential channel giving > > > > events/in_altvoltage0_thresh_falling_value > > events/in_altvoltage1_thresh_falling_value (these match) > > events/in_altvoltage0_thresh_rising_value > > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > > events/in_altvoltage1-altvoltage0_mag_rising_value > > > > Does that work here? Avoids smashing different types of signals together. > > We could even do the LOT as differential between two angle channels > > (tracking one and measured one) but meh that's getting complex.> > > Note this will rely on channel labels to make the above make any sense at all. > > I think this could be OK - I think what matters most is having some > documentation that maps the faults and registers on the chip to the > iio names. Where would the sine/cosine clipping fault fit in though? I > got a bit too creative and used X_OR_Y to differentiate it (see > discussion in "staging: iio: resolver: ad2s1210: implement fault > events"). Strictly speaking, it should probably be a type: threshold, > direction: either event on both the sine and cosine input channels > (another double event) since it occurs if either of the signal exceeds > the power or ground rail voltage. But we already have threshold rising > and threshold falling on these channels with a different meaning. I > guess it could call it magnitude instead of a threshold? Tricky indeed. Though I guess we only hit the clipping case after LOS or DOS fires or if their thresholds are set too wide (is that even possible?). So it is useful to report it as we are already in error? Or can we combine the cases by treating it as a cap on the threshold controls for LOS and DOS? Even when they aren't just there for error reporting, designers seem to always come up with new create signals to use for event detection and sometimes it's a real struggle to map them to something general. Jonathan
On Mon, 2 Oct 2023 11:58:17 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Sat, Sep 30, 2023 at 11:00 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 29 Sep 2023 12:23:31 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > From: David Lechner <david@lechnology.com> > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > When reading the position and velocity on the AD2S1210, there is also a > > > 3rd byte following the two data bytes that contains the fault flag bits. > > > This patch adds support for reading this byte and generating events when > > > faults occur. > > > > > > The faults are mapped to various channels and event types in order to > > > have a unique event for each fault. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > Use of x and y modifiers is a little odd. What was your reasoning? > > Was it just that there was a X_OR_Y modifier? If so, don't use that! > > It seemed like a good idea at the time, but it's not nice to deal with > > and requires a channel with that modifier to hang the controls off > > + make sure userspace expects that event code. > > > Regarding the point about "requires a channel with that modifier to > hang the controls off...". Although that comment was about modifiers, > does it also apply in general. > > There are several fault events that don't have any configurable > parameters, namely _sine/cosine inputs clipping_ and _velocity exceeds > max tracking rate_. So there won't be any attributes that contain the > event specification for those (e.g. no `events/in_angl0_*` > attributes). It sounds like this would be a problem as well? It's fine to have a channel that doesn't have controls or the ability to be read. We do have history of doing what you have here (a couple of accelerometers do it) but it's esoteric and rather hard for userspace to comprehend so I'd rather not introduce it for other types of devices. I think we should go with the most flexible option of allowing events to trigger when they 'may be true' to incorporate this case. Unfortunately I can't see another option that would scale to all the random combinations of events that might occur. There are all sorts of extensions we could make to the event descriptions, but only at the cost of breaking backwards compatibility and simplicity. SWith hindsight the whole IIO_MOD_X_OR_Y_OR_Z mess was a design error :( We can teach userspace code about that quirk for accelerations where the one that would be hard to handle is the AND case used for freefall detectors (you detect that the signal magnitude is near 0 for all axes). I can't think of another option for that one other than the weird modifier (unlike this case) > > Should we consider a IIO_EV_INFO_LABEL so that we can have some sort > of attribute (namely `events/<dir>_<channel spec>_label`) so that > userspace can enumerate expected events for non-configurable events? Probably needs something similar to channel labelling, so a separate callback given we don't handle strings, but sure something like this would be useful and provide 'hints' along the lines of what the datasheet calls a particular event. Not however for what event is sent as such info should be apparent from the event naming. Jonathan
On Thu, Oct 5, 2023 at 9:37 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 2 Oct 2023 11:09:11 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Fri, 29 Sep 2023 12:23:27 -0500 > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > > > From: David Lechner <david@lechnology.com> > > > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > > > 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. > > > > > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > > > error conditions onto one channel. Fundamentally we have > > > sine and cosine inputs. I think we should treat those as separate channels > > > and include a third differential channel between them. > > > > At first, I did consider a differential channel as you suggested in > > v2. However, the datasheet is quite clear that the LOS and DOS faults > > (and only those faults) come from a signal it calls the "monitor > > signal". This signal is defined as: > > > > Monitor = A1 * sin(theta) * sin(phi) + A2 * cos(theta) * cos(phi) > > > > where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the > > cosine input and phi is the position output. So mathematically > > speaking, there is no signal that is the difference between the two > > inputs. (See "Theory of Operation" section in the datasheet.) > > Hmm. That's certainly a bit more complex than I expected. > Relying on the brief description led me astray. > > It's related to the differences in the measured and as if > theta == phi and A1 == A2 (ideal) then it will be A1. > > I can see it's relevant to DOS, but not LOS. The description of LOS > seems to overlap a number of different things unfortunately. > One thing to watch out for in the datasheet is the difference between the fault output pins and the fault bits read over the bus. The LOS output pin does indicate one or more of multiple faults, but we are not currently using that. We are only looking at the fault bits which are more granular. > > > > > > But if we want to hide these internal details and don't care about a > > strict definition of "differential", then what is suggested below > > seems fine. > > Probably best to introduce that monitor signal though we'll have > to be a bit vague about what it is which has the side effect that > anyone trying to understand what on earth these faults are is going > to be confused (having read the datasheet section a couple of times > I'm not 100% sure...) > > > > > > > > > So this one becomes a double event (you need to signal it on both > > > cosine and sine channels). The DOS overange is similar. > > > The DOS mismatch is a threshold on the differential channel giving > > > > > > events/in_altvoltage0_thresh_falling_value > > > events/in_altvoltage1_thresh_falling_value (these match) > > > events/in_altvoltage0_thresh_rising_value > > > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > > > events/in_altvoltage1-altvoltage0_mag_rising_value > > > > > > Does that work here? Avoids smashing different types of signals together. > > > We could even do the LOT as differential between two angle channels > > > (tracking one and measured one) but meh that's getting complex.> > > > Note this will rely on channel labels to make the above make any sense at all. > > > > I think this could be OK - I think what matters most is having some > > documentation that maps the faults and registers on the chip to the > > iio names. Where would the sine/cosine clipping fault fit in though? I > > got a bit too creative and used X_OR_Y to differentiate it (see > > discussion in "staging: iio: resolver: ad2s1210: implement fault > > events"). Strictly speaking, it should probably be a type: threshold, > > direction: either event on both the sine and cosine input channels > > (another double event) since it occurs if either of the signal exceeds > > the power or ground rail voltage. But we already have threshold rising > > and threshold falling on these channels with a different meaning. I > > guess it could call it magnitude instead of a threshold? > > Tricky indeed. Though I guess we only hit the clipping case after > LOS or DOS fires or if their thresholds are set too wide (is that > even possible?). I suppose it _could_ be possible on the high side if the AVDD voltage supply was selected to be less than the 4.4V max of the threshold voltage registers. > So it is useful to report it as we are already in > error? Or can we combine the cases by treating it as a cap on the > threshold controls for LOS and DOS? I found the clipping error useful while developing this driver since it help identify that we had a gain setting wrong on the excitation output (on the circuit board) which in turn caused the inputs to be overdriven. But, yes when this happened, it also always triggered at least one or more of the LOS and DOS faults as well. > > Even when they aren't just there for error reporting, designers > seem to always come up with new create signals to use for event > detection and sometimes it's a real struggle to map them to > something general. > > Jonathan > >
On Mon, 2023-10-02 at 10:17 +0100, Jonathan Cameron wrote: > Hmm. What happened to roadtest? I was hoping that would solve this sort > of issue by allowing simple testing of basic functionality... Roadtest is alive and well. Several of my coworkers have been using it for development and testing of new drivers[0][1][2][3][4] and patches[5][6], and this has resulted in easier testing and refactoring during development, more robust code, and of course the ability to easily detect regressions after the patches are merged. [0] https://lore.kernel.org/lkml/20230323-add-opt4001-driver-v2-2-0bae0398669d@axis.com/ [1] https://lore.kernel.org/lkml/d218a1bc75402b5ebd6e12a563f7315f83fe966c.1689753076.git.waqar.hameed@axis.com/ [2] https://lore.kernel.org/lkml/7b856b74c4c0f8c6c539d7c692051c9203b103c0.1692699931.git.waqar.hameed@axis.com/ [3] https://lore.kernel.org/lkml/20231002-rx8111-add-timestamp0-v1-1-353727cf7f14@axis.com/ [4] https://lore.kernel.org/lkml/20230502-tps6287x-driver-v3-2-e25140a023f5@axis.com/ [5] https://lore.kernel.org/lkml/20221012102347.153201-1-chenhuiz@axis.com/ [6] https://lore.kernel.org/lkml/20220413114014.2204623-3-camel.guo@axis.com/ In fact, by running our roadtests on newer kernels we have found numerous bugs[10][12][14] and regressions[7][8][9][11][15] in mainline, including subsystem-level issues affecting other drivers too. [7] https://lore.kernel.org/lkml/20230911-regulator-voltage-sel-v1-1-886eb1ade8d8@axis.com/ [8] https://lore.kernel.org/lkml/20230918-power-uaf-v1-1-73c397178c42@axis.com/ [9] https://lore.kernel.org/lkml/20230829-tps-voltages-v1-1-7ba4f958a194@axis.com/ [10] https://lore.kernel.org/lkml/20230613-genirq-nested-v3-1-ae58221143eb@axis.com/ [11] https://lore.kernel.org/lkml/20220503114333.456476-1-camel.guo@axis.com/ [12] https://lore.kernel.org/linux-iio/20220816080828.1218667-1-vincent.whitchurch@axis.com/ [13] https://lore.kernel.org/linux-iio/20220519091925.1053897-1-vincent.whitchurch@axis.com/ [14] https://lore.kernel.org/linux-iio/20220620144231.GA23345@axis.com/ [15] https://lore.kernel.org/linux-spi/YxBX4bXG02E4lSUW@axis.com/ (The above lists are not exhaustive.) > Hope it is still headed for a new version / upstream! I pushed out an update with a squash of (most parts of) our internal version out to the following repo, it's based on v6.6-rc4. https://github.com/vwax/linux/tree/roadtest/devel (There are currently 6 lines of --diff-filter=M against v6.6-rc4 on the linked repo. Two of those are from a patch which is posted and waiting for review on the lists, and the rest are for enabling regmap debugfs writes which are used from some of the newer tests.) Since roadtest itself does not require any patches to the kernel or any out-of-tree modules, the maintenance of the framework would not really be simplified by putting it in the upstream tree. However, there is of course a potentially large benefit to the quality of many kinds of kernel drivers if roadtest gets used by others, and having it in-tree could facilitate that. And it would potentially allow regressions like the ones we're finding to be caught _before_ they go in, since anyone can run the tests without special hardware. The idea of having to maintain it in-tree and doing all the work that goes along with that (dealing with the expectations of maintainers, wrangling patches from mailing lists, etc), is something I personally have had a hard time warming up to, but I have some coworkers who may potentially be interested in that kind of work, so I wouldn't rule out another posting of the patch set targeting upstream sometime in the future.
On Fri, 6 Oct 2023 14:48:29 +0000 Vincent Whitchurch <Vincent.Whitchurch@axis.com> wrote: Hi Vincent Thanks for the update, > On Mon, 2023-10-02 at 10:17 +0100, Jonathan Cameron wrote: > > Hmm. What happened to roadtest? I was hoping that would solve this sort > > of issue by allowing simple testing of basic functionality... > > Roadtest is alive and well. Several of my coworkers have been using it > for development and testing of new drivers[0][1][2][3][4] and > patches[5][6], and this has resulted in easier testing and refactoring > during development, more robust code, and of course the ability to > easily detect regressions after the patches are merged. > > [0] https://lore.kernel.org/lkml/20230323-add-opt4001-driver-v2-2-0bae0398669d@axis.com/ > [1] https://lore.kernel.org/lkml/d218a1bc75402b5ebd6e12a563f7315f83fe966c.1689753076.git.waqar.hameed@axis.com/ > [2] https://lore.kernel.org/lkml/7b856b74c4c0f8c6c539d7c692051c9203b103c0.1692699931.git.waqar.hameed@axis.com/ > [3] https://lore.kernel.org/lkml/20231002-rx8111-add-timestamp0-v1-1-353727cf7f14@axis.com/ > [4] https://lore.kernel.org/lkml/20230502-tps6287x-driver-v3-2-e25140a023f5@axis.com/ > [5] https://lore.kernel.org/lkml/20221012102347.153201-1-chenhuiz@axis.com/ > [6] https://lore.kernel.org/lkml/20220413114014.2204623-3-camel.guo@axis.com/ > > In fact, by running our roadtests on newer kernels we have found > numerous bugs[10][12][14] and regressions[7][8][9][11][15] in mainline, > including subsystem-level issues affecting other drivers too. > > [7] https://lore.kernel.org/lkml/20230911-regulator-voltage-sel-v1-1-886eb1ade8d8@axis.com/ > [8] https://lore.kernel.org/lkml/20230918-power-uaf-v1-1-73c397178c42@axis.com/ > [9] https://lore.kernel.org/lkml/20230829-tps-voltages-v1-1-7ba4f958a194@axis.com/ > [10] https://lore.kernel.org/lkml/20230613-genirq-nested-v3-1-ae58221143eb@axis.com/ > [11] https://lore.kernel.org/lkml/20220503114333.456476-1-camel.guo@axis.com/ > [12] https://lore.kernel.org/linux-iio/20220816080828.1218667-1-vincent.whitchurch@axis.com/ > [13] https://lore.kernel.org/linux-iio/20220519091925.1053897-1-vincent.whitchurch@axis.com/ > [14] https://lore.kernel.org/linux-iio/20220620144231.GA23345@axis.com/ > [15] https://lore.kernel.org/linux-spi/YxBX4bXG02E4lSUW@axis.com/ > > (The above lists are not exhaustive.) > Great stuff! > > Hope it is still headed for a new version / upstream! > > I pushed out an update with a squash of (most parts of) our internal > version out to the following repo, it's based on v6.6-rc4. > > https://github.com/vwax/linux/tree/roadtest/devel Thanks. > > (There are currently 6 lines of --diff-filter=M against v6.6-rc4 on the > linked repo. Two of those are from a patch which is posted and waiting > for review on the lists, and the rest are for enabling regmap debugfs > writes which are used from some of the newer tests.) > > Since roadtest itself does not require any patches to the kernel or any > out-of-tree modules, the maintenance of the framework would not really > be simplified by putting it in the upstream tree. However, there is of > course a potentially large benefit to the quality of many kinds of > kernel drivers if roadtest gets used by others, and having it in-tree > could facilitate that. And it would potentially allow regressions like > the ones we're finding to be caught _before_ they go in, since anyone > can run the tests without special hardware. Exactly - my main interest is the dream of getting to the point where new drivers typically also come with roadtest tests, with the aim that they will be used for regression testing. For IIO I might lean on / ask nicely few of the bigger contributors to add fairly comprehensive tests for say one in 3 of their drivers, providing a canary for any subsystem level problems that might sneak in. The stability gained for those drivers might also prove it's own benefit to push people to add tests. At somepoint in the longer term I might even make it a requirement for upstreaming a new driver + slowly tackle the backlog of existing ones. From my experiments with it last year, this is a trivial burden fo > > The idea of having to maintain it in-tree and doing all the work that > goes along with that (dealing with the expectations of maintainers, > wrangling patches from mailing lists, etc), is something I personally > have had a hard time warming up to, but I have some coworkers who may > potentially be interested in that kind of work, so I wouldn't rule out > another posting of the patch set targeting upstream sometime in the > future. I fully appreciate your concern. I just really like roadtest and want a smooth way to integrate using it with my upstream maintenance (and occasional development) process... I of course can't expect you to commit to anything though - I'd be delighted if someone else wants to take this forwards but that would be very much their decision to make! Having not yet waded into the latest code, how 'stable' is it from the point of view of modifications to tests? I can rebase the ones I have out of tree and see, but I'm after an assessment that incorporates what you are planning to change in future. I guess the nasty stuff is if you have a few hundred additional drivers in the test set, any modification to the way they interact with the core of roadtest becomes very painful to push into those tests. One starting point would be to separate the tests directory from the directories containing roadtest frameworks etc as that would help to limit scope of responsibility. If a potential upstream roadtest maintainer is primarily concerned about review + handling of the actual tests, other than potentially letting in some ugly code, I'd imagine any subsystem maintainer who opts into this will take that burden on - perhaps with the occasional question heading your way. I'd certainly not expect you to have to deal with high patch flows and would ensure that didn't happen for any IIO tests (any review people have time for is of course welcome!) +CC a few maintainers of other subsystems who may be interested (I know one of them is ;) Jonathan
From: David Lechner <david@lechnology.com> v3 changes: * Added description of A0/A1 lines in DT bindings. * Added power supply regulators to DT bindings. * Dropped "staging: iio: Documentation: document IIO resolver AD2S1210 sysfs attributes" (these attributes are being removed instead). * Dropped applied patches: * "staging: iio: resolver: ad2s1210: fix ad2s1210_show_fault" * "iio: adc: MCP3564: fix the static checker warning" * Split "staging: iio: resolver: ad2s1210: fix probe" into multiple patches. * Moved sorting imports to separate patch. * Renamed fclkin to clkin_hz. * Added __be16 sample field to state struct for reading raw samples. * Split out new function ad2s1210_single_conversion() from ad2s1210_read_raw(). * Split out new ad2s1210_get_hysteresis() and ad2s1210_set_hysteresis() functions. * Fixed multi-line comment style. * Added notes about soft reset not resetting config registers. * Made use of FIELD_PREP() macro. * Added more explanation to regmap commit message. * Removed datasheet names from channel specs. * Replaced "staging: iio: resolver: ad2s1210: rename fexcit attribute" with "staging: iio: resolver: ad2s1210: convert fexcit to channel attribute". * Replaced "staging: iio: resolver: ad2s1210: add phase_lock_range attributes" with "staging: iio: resolver: ad2s1210: add phase lock range support" * Added additional patches to convert custom device attributes to event attributes. * Added patch to add channel label attributes. v2 changes: * Address initial device tree patch feedback * Drop "iio: sysfs: add IIO_DEVICE_ATTR_NAMED_RW macro" (related cleanups also dropped for now, will address in a future series if needed) * Apply improvements as a series of patches to the staging driver. It is not quite ready for the move out of staging patch yet. This series has been tested on actual hardware using a EVAL-AD2S1210 evaluation board. (Note: not all device tree features have been implemented in the driver since the eval board doesn't support them out of the box. We plan to add them later if needed.) Most of the questions about dealing with faults from the v2 cover letter have been addressed. There is still the question about what to do with the current `fault` attribute (it is the only custom device attribute remaining from the original staging driver). It was suggested to split it out into multiple attributes in a subdirectory. Since we now have events for all of the faults, I'm wondering if this is something that is still needed. In the current implementation, it is possible to start listening to events, clear the faults and then read a sample to trigger events for any current faults so we have a way to get current faults already. There is also the matter of clearing faults. Writing the excitation frequency has a side-effect of clearing the faults, so we could use that as the reset. Or we could change the current fault attribute to write-only and rename it. Or is there a better way that I have overlooked? Once this last issue is addressed, I think this driver will be ready for consideration for moving out of staging. --- David Lechner (27): dt-bindings: iio: resolver: add devicetree bindings for ad2s1210 staging: iio: resolver: ad2s1210: fix use before initialization staging: iio: resolver: ad2s1210: remove call to spi_setup() staging: iio: resolver: ad2s1210: check return of ad2s1210_initial() staging: iio: resolver: ad2s1210: remove spi_set_drvdata() staging: iio: resolver: ad2s1210: sort imports staging: iio: resolver: ad2s1210: always use 16-bit value for raw read staging: iio: resolver: ad2s1210: implement IIO_CHAN_INFO_SCALE staging: iio: resolver: ad2s1210: use devicetree to get CLKIN rate staging: iio: resolver: ad2s1210: use regmap for config registers staging: iio: resolver: ad2s1210: add debugfs reg access staging: iio: resolver: ad2s1210: remove config attribute staging: iio: resolver: ad2s1210: rework gpios staging: iio: resolver: ad2s1210: implement hysteresis as channel attr staging: iio: resolver: ad2s1210: refactor setting excitation frequency staging: iio: resolver: ad2s1210: read excitation frequency from control register staging: iio: resolver: ad2s1210: convert fexcit to channel attribute staging: iio: resolver: ad2s1210: convert resolution to devicetree property staging: iio: resolver: ad2s1210: add phase lock range support staging: iio: resolver: ad2s1210: add triggered buffer support staging: iio: resolver: ad2s1210: convert LOT threshold attrs to event attrs staging: iio: resolver: ad2s1210: convert LOS threshold to event attr staging: iio: resolver: ad2s1210: convert DOS overrange threshold to event attr staging: iio: resolver: ad2s1210: convert DOS mismatch threshold to event attr staging: iio: resolver: ad2s1210: rename DOS reset min/max attrs staging: iio: resolver: ad2s1210: implement fault events staging: iio: resolver: ad2s1210: add label attribute support .../bindings/iio/resolver/adi,ad2s1210.yaml | 177 +++ .../Documentation/sysfs-bus-iio-resolver-ad2s1210 | 27 + drivers/staging/iio/resolver/Kconfig | 1 + drivers/staging/iio/resolver/ad2s1210.c | 1583 +++++++++++++++----- 4 files changed, 1391 insertions(+), 397 deletions(-) --- base-commit: 5e99f692d4e32e3250ab18d511894ca797407aec change-id: 20230925-ad2s1210-mainline-2791ef75e386