Messages in this thread | | | Subject | Re: [PATCH v3 2/4] iio: accel: adxl372: add event interface | From | Lars-Peter Clausen <> | Date | Wed, 18 Mar 2020 11:06:55 +0100 |
| |
On 3/18/20 12:09 PM, Alexandru Tachici wrote: > Currently the driver configures adxl372 to work in loop mode. > The inactivity and activity timings decide how fast the chip > will loop through the awake and waiting states and the > thresholds on x,y,z axis decide when activity or inactivity > will be detected. > > This patch adds standard events sysfs entries for the inactivity > and activity timings: thresh_falling_period/thresh_rising_period > and for the in_accel_x_thresh_falling/rising_value. > > Signed-off-by: Alexandru Tachici <alexandru.tachici@analog.com>
Hi,
Looks very good.
Since you are sending this from your gmail account there is a mismatch between the driver author and the signed-off-by. Either Add "From: Alexandru Tachici <alexandru.tachici@analog.com>" to the first line of the patch description or sign-off with your gmail address.
> --- [...] > #define ADXL372_ACCEL_CHANNEL(index, reg, axis) { \ > .type = IIO_ACCEL, \ > .address = reg, \ > @@ -241,6 +271,8 @@ static const struct adxl372_axis_lookup adxl372_axis_lookup_table[] = { > .storagebits = 16, \ > .shift = 4, \ > }, \ > + .event_spec = adxl372_events, \ > + .num_event_specs = 2 \
ARRAY_SIZE(adxl372_events)
> } > > static const struct iio_chan_spec adxl372_channels[] = { > @@ -280,6 +312,43 @@ static const unsigned long adxl372_channel_masks[] = { > 0 > }; > > +static ssize_t adxl372_read_threshold_value(struct iio_dev *indio_dev, > + unsigned int addr, > + u16 *threshold) > +{ > + struct adxl372_state *st = iio_priv(indio_dev); > + __be16 __regval;
I'd avoid using the __ prefix for normal identifiers as those are supposed to be reserved. Maybe use raw_regval or something like that.
> + u16 regval; > + int ret; > + > + ret = regmap_bulk_read(st->regmap, addr, &__regval, sizeof(__regval)); > + if (ret < 0) > + return ret; > + > + regval = be16_to_cpu(__regval); > + regval >>= 5; > + > + *threshold = regval; > + > + return 0; > +} > + > +static ssize_t adxl372_write_threshold_value(struct iio_dev *indio_dev, > + unsigned int addr, > + u16 threshold) > +{ > + struct adxl372_state *st = iio_priv(indio_dev); > + int ret; > + > + ret = regmap_write(st->regmap, addr, > + ADXL372_THRESH_VAL_H_SEL(threshold)); > + if (ret < 0) > + return ret; > + > + return regmap_update_bits(st->regmap, addr + 1, GENMASK(7, 5), > + ADXL372_THRESH_VAL_L_SEL(threshold) << 5);
I think this needs a lock to make sure that the update to the two registers is atomic.
> +} > + > static int adxl372_read_axis(struct adxl372_state *st, u8 addr) > { > __be16 regval; > @@ -543,6 +612,27 @@ static void adxl372_arrange_axis_data(struct adxl372_state *st, __be16 *sample) > memcpy(sample, axis_sample, 3 * sizeof(__be16)); > } > > +static void adxl372_push_event(struct iio_dev *indio_dev, s64 timestamp, > + u8 status2) > +{ > + unsigned int ev_dir;
= IIO_EV_DIR_NONE;
Otherwise it is uninitialized and you might send spurious events.
> + > + if (ADXL372_STATUS_2_ACT(status2)) > + ev_dir = IIO_EV_DIR_RISING; > + > + if (ADXL372_STATUS_2_INACT(status2)) > + ev_dir = IIO_EV_DIR_FALLING; > + > + if (ev_dir != IIO_EV_DIR_NONE) > + iio_push_event(indio_dev, > + IIO_MOD_EVENT_CODE(IIO_ACCEL, > + 0, > + IIO_MOD_X_OR_Y_OR_Z, > + IIO_EV_TYPE_THRESH, > + ev_dir), > + timestamp); > +} [...]
| |