lkml.org 
[lkml]   [2020]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 2/4] iio: accel: adxl372: add event interface
    From
    Date
    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);
    > +}
    [...]

    \
     
     \ /
      Last update: 2020-03-18 11:07    [W:3.373 / U:0.100 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site