| Message ID | 20230929-ad2s1210-mainline-v3-26-fa4364281745@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: resolver: move ad2s1210 out of staging | expand |
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-26-fa4364281745%40baylibre.com
patch subject: [PATCH v3 26/27] staging: iio: resolver: ad2s1210: implement fault events
config: i386-randconfig-003-20230930 (https://download.01.org/0day-ci/archive/20230930/202309301112.XWUnLqJ3-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230930/202309301112.XWUnLqJ3-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309301112.XWUnLqJ3-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/staging/iio/resolver/ad2s1210.c:1452:34: warning: 'ad2s1210_of_match' defined but not used [-Wunused-const-variable=]
1452 | static const struct of_device_id ad2s1210_of_match[] = {
| ^~~~~~~~~~~~~~~~~
drivers/staging/iio/resolver/ad2s1210.c: In function 'ad2s1210_read_raw':
>> drivers/staging/iio/resolver/ad2s1210.c:436:40: warning: array subscript 2 is above array bounds of 'u8[2]' {aka 'unsigned char[2]'} [-Warray-bounds]
436 | ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
| ~~~~~~^~~
vim +436 drivers/staging/iio/resolver/ad2s1210.c
390
391 static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
392 struct iio_chan_spec const *chan,
393 int *val)
394 {
395 struct ad2s1210_state *st = iio_priv(indio_dev);
396 s64 timestamp;
397 int ret;
398
399 mutex_lock(&st->lock);
400 gpiod_set_value(st->sample_gpio, 1);
401 timestamp = iio_get_time_ns(indio_dev);
402 /* delay (6 * tck + 20) nano seconds */
403 udelay(1);
404
405 switch (chan->type) {
406 case IIO_ANGL:
407 ret = ad2s1210_set_mode(st, MOD_POS);
408 break;
409 case IIO_ANGL_VEL:
410 ret = ad2s1210_set_mode(st, MOD_VEL);
411 break;
412 default:
413 ret = -EINVAL;
414 break;
415 }
416 if (ret < 0)
417 goto error_ret;
418 ret = spi_read(st->sdev, &st->sample, 3);
419 if (ret < 0)
420 goto error_ret;
421
422 switch (chan->type) {
423 case IIO_ANGL:
424 *val = be16_to_cpu(st->sample.raw);
425 ret = IIO_VAL_INT;
426 break;
427 case IIO_ANGL_VEL:
428 *val = (s16)be16_to_cpu(st->sample.raw);
429 ret = IIO_VAL_INT;
430 break;
431 default:
432 ret = -EINVAL;
433 break;
434 }
435
> 436 ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
437
438 error_ret:
439 gpiod_set_value(st->sample_gpio, 0);
440 /* delay (2 * tck + 20) nano seconds */
441 udelay(1);
442 mutex_unlock(&st->lock);
443 return ret;
444 }
445
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 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 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.
Hi David,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-26-fa4364281745%40baylibre.com
patch subject: [PATCH v3 26/27] staging: iio: resolver: ad2s1210: implement fault events
config: x86_64-randconfig-161-20231002 (https://download.01.org/0day-ci/archive/20231003/202310031839.tKR53HoP-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231003/202310031839.tKR53HoP-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202310031839.tKR53HoP-lkp@intel.com/
smatch warnings:
drivers/staging/iio/resolver/ad2s1210.c:436 ad2s1210_single_conversion() error: buffer overflow 'st->rx' 2 <= 2
vim +436 drivers/staging/iio/resolver/ad2s1210.c
ecf16f4922f691 David Lechner 2023-09-29 391 static int ad2s1210_single_conversion(struct iio_dev *indio_dev,
29148543c52146 Jonathan Cameron 2011-10-05 392 struct iio_chan_spec const *chan,
e9336a85ceb885 David Lechner 2023-09-29 393 int *val)
817e5c65c511d4 Graf Yang 2010-10-27 394 {
ecf16f4922f691 David Lechner 2023-09-29 395 struct ad2s1210_state *st = iio_priv(indio_dev);
ecf16f4922f691 David Lechner 2023-09-29 396 s64 timestamp;
69cc7fbdcdf2e3 David Lechner 2023-09-29 397 int ret;
817e5c65c511d4 Graf Yang 2010-10-27 398
817e5c65c511d4 Graf Yang 2010-10-27 399 mutex_lock(&st->lock);
69cc7fbdcdf2e3 David Lechner 2023-09-29 400 gpiod_set_value(st->sample_gpio, 1);
ecf16f4922f691 David Lechner 2023-09-29 401 timestamp = iio_get_time_ns(indio_dev);
817e5c65c511d4 Graf Yang 2010-10-27 402 /* delay (6 * tck + 20) nano seconds */
817e5c65c511d4 Graf Yang 2010-10-27 403 udelay(1);
817e5c65c511d4 Graf Yang 2010-10-27 404
29148543c52146 Jonathan Cameron 2011-10-05 405 switch (chan->type) {
29148543c52146 Jonathan Cameron 2011-10-05 406 case IIO_ANGL:
69cc7fbdcdf2e3 David Lechner 2023-09-29 407 ret = ad2s1210_set_mode(st, MOD_POS);
29148543c52146 Jonathan Cameron 2011-10-05 408 break;
29148543c52146 Jonathan Cameron 2011-10-05 409 case IIO_ANGL_VEL:
69cc7fbdcdf2e3 David Lechner 2023-09-29 410 ret = ad2s1210_set_mode(st, MOD_VEL);
29148543c52146 Jonathan Cameron 2011-10-05 411 break;
29148543c52146 Jonathan Cameron 2011-10-05 412 default:
29148543c52146 Jonathan Cameron 2011-10-05 413 ret = -EINVAL;
29148543c52146 Jonathan Cameron 2011-10-05 414 break;
29148543c52146 Jonathan Cameron 2011-10-05 415 }
29148543c52146 Jonathan Cameron 2011-10-05 416 if (ret < 0)
29148543c52146 Jonathan Cameron 2011-10-05 417 goto error_ret;
ecf16f4922f691 David Lechner 2023-09-29 418 ret = spi_read(st->sdev, &st->sample, 3);
29148543c52146 Jonathan Cameron 2011-10-05 419 if (ret < 0)
817e5c65c511d4 Graf Yang 2010-10-27 420 goto error_ret;
29148543c52146 Jonathan Cameron 2011-10-05 421
29148543c52146 Jonathan Cameron 2011-10-05 422 switch (chan->type) {
29148543c52146 Jonathan Cameron 2011-10-05 423 case IIO_ANGL:
ecf16f4922f691 David Lechner 2023-09-29 424 *val = be16_to_cpu(st->sample.raw);
29148543c52146 Jonathan Cameron 2011-10-05 425 ret = IIO_VAL_INT;
29148543c52146 Jonathan Cameron 2011-10-05 426 break;
29148543c52146 Jonathan Cameron 2011-10-05 427 case IIO_ANGL_VEL:
ecf16f4922f691 David Lechner 2023-09-29 428 *val = (s16)be16_to_cpu(st->sample.raw);
29148543c52146 Jonathan Cameron 2011-10-05 429 ret = IIO_VAL_INT;
29148543c52146 Jonathan Cameron 2011-10-05 430 break;
29148543c52146 Jonathan Cameron 2011-10-05 431 default:
5e99f692d4e32e David Lechner 2023-09-21 432 ret = -EINVAL;
5e99f692d4e32e David Lechner 2023-09-21 433 break;
29148543c52146 Jonathan Cameron 2011-10-05 434 }
29148543c52146 Jonathan Cameron 2011-10-05 435
ecf16f4922f691 David Lechner 2023-09-29 @436 ad2s1210_push_events(indio_dev, st->rx[2], timestamp);
^^^^^^
Apparently ->rx only has 2 elements.
ecf16f4922f691 David Lechner 2023-09-29 437
817e5c65c511d4 Graf Yang 2010-10-27 438 error_ret:
69cc7fbdcdf2e3 David Lechner 2023-09-29 439 gpiod_set_value(st->sample_gpio, 0);
817e5c65c511d4 Graf Yang 2010-10-27 440 /* delay (2 * tck + 20) nano seconds */
817e5c65c511d4 Graf Yang 2010-10-27 441 udelay(1);
817e5c65c511d4 Graf Yang 2010-10-27 442 mutex_unlock(&st->lock);
29148543c52146 Jonathan Cameron 2011-10-05 443 return ret;
817e5c65c511d4 Graf Yang 2010-10-27 444 }
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
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index e1c95ec73545..dc3cc3ab855e 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -21,6 +21,7 @@ #include <linux/types.h> #include <linux/iio/buffer.h> +#include <linux/iio/events.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> #include <linux/iio/trigger_consumer.h> @@ -35,6 +36,16 @@ #define AD2S1210_SET_ENRES GENMASK(3, 2) #define AD2S1210_SET_RES GENMASK(1, 0) +/* fault register flags */ +#define AD2S1210_FAULT_CLIP BIT(7) +#define AD2S1210_FAULT_LOS BIT(6) +#define AD2S1210_FAULT_DOS_OVR BIT(5) +#define AD2S1210_FAULT_DOS_MIS BIT(4) +#define AD2S1210_FAULT_LOT BIT(3) +#define AD2S1210_FAULT_VELOCITY BIT(2) +#define AD2S1210_FAULT_PHASE BIT(1) +#define AD2S1210_FAULT_CONFIG_PARITY BIT(0) + #define AD2S1210_REG_POSITION_MSB 0x80 #define AD2S1210_REG_POSITION_LSB 0x81 #define AD2S1210_REG_VELOCITY_MSB 0x82 @@ -71,6 +82,8 @@ /* max voltage for threshold registers is 0x7F * 38 mV */ #define THRESHOLD_RANGE_STR "[0 38 4826]" +#define FAULT_ONESHOT(bit, new, old) (new & bit && !(old & bit)) + enum ad2s1210_mode { MOD_POS = 0b00, MOD_VEL = 0b01, @@ -98,8 +111,13 @@ struct ad2s1210_state { unsigned long clkin_hz; /** The selected resolution */ enum ad2s1210_resolution resolution; + /** Copy of fault register from the previous read. */ + u8 prev_fault_flags; /** For reading raw sample value via SPI. */ - __be16 sample __aligned(IIO_DMA_MINALIGN); + struct { + __be16 raw; + u8 fault; + } sample __aligned(IIO_DMA_MINALIGN);; /** Scan buffer */ struct { __be16 chan[2]; @@ -158,7 +176,15 @@ static int ad2s1210_regmap_reg_write(void *context, unsigned int reg, if (ret < 0) return ret; - return spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers)); + ret = spi_sync_transfer(st->sdev, xfers, ARRAY_SIZE(xfers)); + if (ret < 0) + return ret; + + /* soft reset also clears the fault register */ + if (reg == AD2S1210_REG_SOFT_RESET) + st->prev_fault_flags = 0; + + return 0; } /* @@ -200,6 +226,10 @@ static int ad2s1210_regmap_reg_read(void *context, unsigned int reg, if (ret < 0) return ret; + /* reading the fault register also clears it */ + if (reg == AD2S1210_REG_FAULT) + st->prev_fault_flags = 0; + /* * If the D7 bit is set on any read/write register, it indicates a * parity error. The fault register is read-only and the D7 bit means @@ -283,14 +313,92 @@ static ssize_t ad2s1210_clear_fault(struct device *dev, return ret < 0 ? ret : len; } -static int ad2s1210_single_conversion(struct ad2s1210_state *st, +static void ad2s1210_push_events(struct iio_dev *indio_dev, + u8 flags, s64 timestamp) +{ + struct ad2s1210_state *st = iio_priv(indio_dev); + + /* Sine/cosine inputs clipped */ + if (FAULT_ONESHOT(AD2S1210_FAULT_CLIP, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_MOD_EVENT_CODE(IIO_ALTVOLTAGE, 1, + IIO_MOD_X_OR_Y, + IIO_EV_TYPE_MAG, + IIO_EV_DIR_NONE), + timestamp); + + /* Sine/cosine inputs below LOS threshold */ + if (FAULT_ONESHOT(AD2S1210_FAULT_LOS, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_FALLING), + timestamp); + + /* Sine/cosine inputs exceed DOS overrange threshold */ + if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_OVR, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_RISING), + timestamp); + + /* Sine/cosine inputs exceed DOS mismatch threshold */ + if (FAULT_ONESHOT(AD2S1210_FAULT_DOS_MIS, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_ALTVOLTAGE, 0, + IIO_EV_TYPE_MAG, + IIO_EV_DIR_NONE), + timestamp); + + /* Tracking error exceeds LOT threshold */ + if (FAULT_ONESHOT(AD2S1210_FAULT_LOT, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_ANGL, 1, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_RISING), + timestamp); + + /* Velocity exceeds maximum tracking rate */ + if (FAULT_ONESHOT(AD2S1210_FAULT_VELOCITY, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_ANGL_VEL, 0, + IIO_EV_TYPE_THRESH, + IIO_EV_DIR_RISING), + timestamp); + + /* Phase error exceeds phase lock range */ + if (FAULT_ONESHOT(AD2S1210_FAULT_PHASE, flags, st->prev_fault_flags)) + iio_push_event(indio_dev, + IIO_UNMOD_EVENT_CODE(IIO_PHASE, 0, + IIO_EV_TYPE_MAG, + IIO_EV_DIR_NONE), + timestamp); + + /* Configuration parity error */ + if (FAULT_ONESHOT(AD2S1210_FAULT_CONFIG_PARITY, flags, + st->prev_fault_flags)) + /* + * Userspace should also get notified of this via error return + * when trying to write to any attribute that writes a register. + */ + dev_err_ratelimited(&indio_dev->dev, + "Configuration parity error\n"); + + st->prev_fault_flags = flags; +} + +static int ad2s1210_single_conversion(struct iio_dev *indio_dev, struct iio_chan_spec const *chan, int *val) { + struct ad2s1210_state *st = iio_priv(indio_dev); + s64 timestamp; int ret; mutex_lock(&st->lock); gpiod_set_value(st->sample_gpio, 1); + timestamp = iio_get_time_ns(indio_dev); /* delay (6 * tck + 20) nano seconds */ udelay(1); @@ -307,17 +415,17 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, } if (ret < 0) goto error_ret; - ret = spi_read(st->sdev, &st->sample, 2); + ret = spi_read(st->sdev, &st->sample, 3); if (ret < 0) goto error_ret; switch (chan->type) { case IIO_ANGL: - *val = be16_to_cpu(st->sample); + *val = be16_to_cpu(st->sample.raw); ret = IIO_VAL_INT; break; case IIO_ANGL_VEL: - *val = (s16)be16_to_cpu(st->sample); + *val = (s16)be16_to_cpu(st->sample.raw); ret = IIO_VAL_INT; break; default: @@ -325,6 +433,8 @@ static int ad2s1210_single_conversion(struct ad2s1210_state *st, break; } + ad2s1210_push_events(indio_dev, st->rx[2], timestamp); + error_ret: gpiod_set_value(st->sample_gpio, 0); /* delay (2 * tck + 20) nano seconds */ @@ -608,7 +718,7 @@ static int ad2s1210_read_raw(struct iio_dev *indio_dev, switch (mask) { case IIO_CHAN_INFO_RAW: - return ad2s1210_single_conversion(st, chan, val); + return ad2s1210_single_conversion(indio_dev, chan, val); case IIO_CHAN_INFO_SCALE: switch (chan->type) { case IIO_ANGL: @@ -721,6 +831,14 @@ static const struct iio_event_spec ad2s1210_position_event_spec[] = { }, }; +static const struct iio_event_spec ad2s1210_velocity_event_spec[] = { + { + /* Velocity exceeds maximum tracking rate fault. */ + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + }, +}; + static const struct iio_event_spec ad2s1210_phase_event_spec[] = { { /* Phase error fault. */ @@ -754,6 +872,14 @@ static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = { }, }; +static const struct iio_event_spec ad2s1210_sin_cos_event_spec[] = { + { + /* Sine/cosine clipping fault. */ + .type = IIO_EV_TYPE_MAG, + .dir = IIO_EV_DIR_NONE, + }, +}; + static const struct iio_chan_spec ad2s1210_channels[] = { { .type = IIO_ANGL, @@ -784,6 +910,8 @@ static const struct iio_chan_spec ad2s1210_channels[] = { }, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), + .event_spec = ad2s1210_velocity_event_spec, + .num_event_specs = ARRAY_SIZE(ad2s1210_velocity_event_spec), }, IIO_CHAN_SOFT_TIMESTAMP(2), { @@ -820,6 +948,26 @@ static const struct iio_chan_spec ad2s1210_channels[] = { .scan_index = -1, .event_spec = ad2s1210_monitor_signal_event_spec, .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec), + }, { + /* sine input */ + .type = IIO_ALTVOLTAGE, + .indexed = 1, + .channel = 1, + .modified = 1, + .channel2 = IIO_MOD_Y, + .scan_index = -1, + .event_spec = ad2s1210_sin_cos_event_spec, + .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec), + }, { + /* cosine input */ + .type = IIO_ALTVOLTAGE, + .indexed = 1, + .channel = 1, + .modified = 1, + .channel2 = IIO_MOD_X, + .scan_index = -1, + .event_spec = ad2s1210_sin_cos_event_spec, + .num_event_specs = ARRAY_SIZE(ad2s1210_sin_cos_event_spec), }, }; @@ -936,7 +1084,7 @@ static const struct attribute_group ad2s1210_event_attribute_group = { static int ad2s1210_initial(struct ad2s1210_state *st) { - unsigned char data; + unsigned int data; int ret; mutex_lock(&st->lock); @@ -1073,12 +1221,11 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p) if (ret < 0) goto error_ret; - /* REVIST: we can read 3 bytes here and also get fault flags */ - ret = spi_read(st->sdev, st->rx, 2); + ret = spi_read(st->sdev, &st->sample, 3); if (ret < 0) goto error_ret; - memcpy(&st->scan.chan[chan++], st->rx, 2); + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2); } if (test_bit(1, indio_dev->active_scan_mask)) { @@ -1086,14 +1233,14 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p) if (ret < 0) goto error_ret; - /* REVIST: we can read 3 bytes here and also get fault flags */ - ret = spi_read(st->sdev, st->rx, 2); + ret = spi_read(st->sdev, &st->sample, 3); if (ret < 0) goto error_ret; - memcpy(&st->scan.chan[chan++], st->rx, 2); + memcpy(&st->scan.chan[chan++], &st->sample.raw, 2); } + ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp); iio_push_to_buffers_with_timestamp(indio_dev, &st->scan, pf->timestamp); error_ret: