On Tue, 11 Mar 2025 11:55:05 +0100
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> Hi Jonathan,
>
> I'm currently about to update the series and like to answer some of
> your review comments directly when submitting. Nevertheless, here I'll
> anticipate this one. Pls, find my questions inlined down below.
>
> On Tue, Mar 4, 2025 at 2:49 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Thu, 20 Feb 2025 10:42:31 +0000
> > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >
> > > Make the sensor detect and issue interrupts at activity. Activity
> > > events are configured by a threshold stored in regmap cache.
> > >
> > > Activity, together with ODR and range setting are preparing a setup
> > > together with inactivity coming in a follow up patch.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > Some questions / comments inline.
> >
> > This review is has been at random moments whilst travelling (hence
> > over several days!) so it may be less than thorough or consistent!
> > I should be back to normal sometime next week.
> >
>
> No problem, no hurry!
>
> > > @@ -258,6 +284,73 @@ static int adxl345_set_measure_en(struct adxl345_state *st, bool en)
> > > return regmap_write(st->regmap, ADXL345_REG_POWER_CTL, val);
> > > }
> > >
> > > +/* act/inact */
> > > +
> > > +static int adxl345_write_act_axis(struct adxl345_state *st,
> > > + enum adxl345_activity_type type, bool en)
> > > +{
> > > + int ret;
> > > +
> > > + /*
> > > + * The ADXL345 allows for individually enabling/disabling axis for
> > > + * activity and inactivity detection, respectively. Here both axis are
> > > + * kept in sync, i.e. an axis will be generally enabled or disabled for
> > > + * both equally, activity and inactivity detection.
> >
> > Why? Can definitely see people only being interested in one case
> > and not the other. What advantage is there in always having both
> > or neither over separate controls?
>
> Ugh! This happens when I mix writing code and writing English texts,
> w/o re-reading it.
>
> Situation: The sensor allows to individually enable / disable x,y and
> z axis for r activity and for inactivity. I don't offer this in the
> driver. When activity is selected, I always enable all three axis or
> disable them. Similar, for inactivity.
Ah. All axis together, not always both inactivty and activity.
> The question is then actually,
> if this is legitimate, or should I really implement enabling/disabling
> of each axis individually for activity and similar then for
> inactivity? I mean, when not interested in one or the other axis,
> someone can fiilter the result.
For some sensors there is no way to distinguish which threshold was hit
(they just provide 1 bit for activity in the status register)
Here it seems we get a single bit that indicates first act or inact
triggering axis (in ACT_TAP_STATUS). Assuming I read that
text correcty only one bit is set. That's not exactly useful as
it doesn't tell use other axis would have triggered it.
So I think here your approach of all axis enable is perhaps the
right approach. Also report it as an X_OR_Y_OR_Z event and ignore
the indication of which axis perhaps?
> On the other side I can imagine a
> small impact in power consumption, when only one axis is used instead
> of three axis (?). Since the ADXL345 is [was] one of Analog's fancy
> acme-ultra-low-power-sensors, power is definitvely a topic here IMHO.
> I can't really estimate the importance.
I doubt it would make a measurable difference to power usage.
>
> I guess I'll try to implement it and see how ugly it gets. At least
> it's a good exercise. As also, I'll try to bring regmap cache and
> clear_bits / set_bits more in here for activity and inactivity in the
> next version.
>
> > > @@ -842,6 +972,23 @@ static int adxl345_write_event_value(struct iio_dev *indio_dev,
> > > return ret;
> > >
> > > switch (type) {
> > > + case IIO_EV_TYPE_THRESH:
> > > + switch (info) {
> > > + case IIO_EV_INFO_VALUE:
> > > + switch (dir) {
> > > + case IIO_EV_DIR_RISING:
> > > + ret = regmap_write(st->regmap,
> > > + adxl345_act_thresh_reg[ADXL345_ACTIVITY],
> > > + val);
> > > + break;
> > This collection of breaks and nested functions suggests maybe we can either
> > return directly (I've lost track of what happens after this) or that
> > we should factor out some of this code to allow direct returns in the
> > function we put that code in. Chasing the breaks is not great if it
> > doesn't lead to anything interesting.
>
> I understand, but since I'm using quite a bit configuration for the
> sensor, I'm taking advantage
> of type, info and dir here. It won't get more complex than that. I'm
> [actually] pretty sure, since this
> then is almost feature complete.
>
> I don't see a different way how to do it. I mean, I could still
> separate handling the "dir" entirely in
> a called function. Or, say, implement IIO_EV_TYPE_THRESH handling in a
> separate function?
> Pls, let me know what you think.
Maybe factor everything out between the set_measure_en and it's counterpart.
Then you can just return in all paths in the factored out function.
That's nice because anyone reading it doesn't have to chase down to
see what else happens.
It might make sense to break it up further but probably not.
>
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > + break;
> > > + default:
> > > + ret = -EINVAL;
> > > + }
> > > + break;
> > > case IIO_EV_TYPE_GESTURE:
> > > switch (info) {
> > > case IIO_EV_INFO_VALUE:
> > > @@ -1124,6 +1271,17 @@ static int adxl345_push_event(struct iio_dev *indio_dev, int int_stat,
> > > return ret;
> > > }
> > >
> > > + if (FIELD_GET(ADXL345_INT_ACTIVITY, int_stat)) {
> > > + ret = iio_push_event(indio_dev,
> > > + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > + act_tap_dir,
> > > + IIO_EV_TYPE_THRESH,
> > > + IIO_EV_DIR_RISING),
> > > + ts);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > if (FIELD_GET(ADXL345_INT_FREE_FALL, int_stat)) {
> > > ret = iio_push_event(indio_dev,
> > > IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
> > > @@ -1157,6 +1315,7 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > > ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> > > if (ret)
> > > return ret;
> > > + /* tap direction */
> >
> > Belongs in earlier patch?
> >
> > > if (FIELD_GET(ADXL345_Z_EN, regval) > 0)
> > > act_tap_dir = IIO_MOD_Z;
> > > else if (FIELD_GET(ADXL345_Y_EN, regval) > 0)
> > > @@ -1165,6 +1324,19 @@ static irqreturn_t adxl345_irq_handler(int irq, void *p)
> > > act_tap_dir = IIO_MOD_X;
> > > }
> > >
> > > + if (FIELD_GET(ADXL345_REG_ACT_AXIS_MSK, st->act_axis_ctrl) > 0) {
> > > + ret = regmap_read(st->regmap, ADXL345_REG_ACT_TAP_STATUS, ®val);
> > > + if (ret)
> > > + return ret;
> > > + /* activity direction */
> > > + if (FIELD_GET(ADXL345_Z_EN, regval >> 4) > 0)
> >
> > I'm not following the shifts here. That looks like we don't have
> > defines that we should but instead use the ones for the lower fields.
>
> The 8-bit register is split as follows:
>
> | 7 | 6 | 5 | 4 | 3
> | 2 | 1 | 0 |
> ----------------------------------------------------------------------------------------------------------------------
> | ACT ac/dc | ACT_X | ACT_Y | ACT_Z | INACT ac/dc | INACT_X | INACT_Y
> | INACT_Z |
>
> I thought here, either I shift the ACT_* directions by 4 then use the
> general mask for axis (lower 4 bits). Or I introduce an axis enum for
> ACT_* and a separate one for INACT_*. Thus, I kept the shift and use
> the same ADXL345_*_EN mask. How can I improve this, or can this stay?
I'd not use enums here at all unless you actually use the enum
type directly somewhere to enforce allowed values.
Separate defines for inact and act make sense though. Saves the reader
of this bit of the code having to care about the layout of the fields,
Jonathan
>
> >
> > > + act_tap_dir = IIO_MOD_Z;
> > > + else if (FIELD_GET(ADXL345_Y_EN, regval >> 4) > 0)
> > > + act_tap_dir = IIO_MOD_Y;
> > > + else if (FIELD_GET(ADXL345_X_EN, regval >> 4) > 0)
> > > + act_tap_dir = IIO_MOD_X;
> > > + }
> >
> >
>
> Best,
> L
