Re: [PATCH v6 4/8] iio: accel: adxl313: add activity sensing

[Jonathan Cameron]

On Tue, 1 Jul 2025 00:32:57 +0200
Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:

> On Sat, Jun 28, 2025 at 7:27 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> >
> > On Sun, 22 Jun 2025 12:29:33 +0000
> > Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
> >  
> > > Add support for configuring an activity detection threshold. Extend the
> > > interrupt handler to process activity-related interrupts, and provide
> > > functions to set the threshold as well as to enable or disable activity
> > > sensing. Additionally, introduce a virtual channel that represents the
> > > logical AND of the x, y, and z axes in the IIO channel.
> > >
> > > This patch serves as a preparatory step; some definitions and functions
> > > introduced here are intended to be extended later to support inactivity
> > > detection.
> > >
> > > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>  
> > Hi Lothar.
> >
> > I think this is suffering from function naming evolution and we need
> > to rethink (slightly) what we ended up with.  See inline.
> > I walked into the same trap recently so was on the look out for it.
> >  
> > > ---
> > >  drivers/iio/accel/adxl313_core.c | 326 +++++++++++++++++++++++++++++++
> > >  1 file changed, 326 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> > > index ac4cc16399fc..d2c625f27555 100644
> > > --- a/drivers/iio/accel/adxl313_core.c
> > > +++ b/drivers/iio/accel/adxl313_core.c
> > > @@ -13,8 +13,10 @@  
> >  
> > > +
> > > +static int _adxl313_read_mag_value(struct adxl313_data *data,
> > > +                                enum iio_event_direction dir,
> > > +                                enum adxl313_activity_type type_act,
> > > +                                int *val, int *val2)
> > > +{
> > > +     unsigned int threshold;
> > > +     int ret;
> > > +
> > > +     switch (dir) {
> > > +     case IIO_EV_DIR_RISING:
> > > +             ret = regmap_read(data->regmap,
> > > +                               adxl313_act_thresh_reg[type_act],
> > > +                               &threshold);
> > > +             if (ret)
> > > +                     return ret;
> > > +             *val = threshold * 15625;
> > > +             *val2 = MICRO;
> > > +             return IIO_VAL_FRACTIONAL;
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static int _adxl313_write_mag_value(struct adxl313_data *data,
> > > +                                 enum iio_event_direction dir,
> > > +                                 enum adxl313_activity_type type_act,
> > > +                                 int val, int val2)
> > > +{
> > > +     unsigned int regval;
> > > +
> > > +     /* Scale factor 15.625 mg/LSB */
> > > +     regval = DIV_ROUND_CLOSEST(MICRO * val + val2, 15625);
> > > +     switch (dir) {
> > > +     case IIO_EV_DIR_RISING:
> > > +             return regmap_write(data->regmap,
> > > +                                 adxl313_act_thresh_reg[type_act],
> > > +                                 regval);
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static int adxl313_read_mag_value(struct adxl313_data *data,
> > > +                               enum iio_event_direction dir,
> > > +                               enum iio_event_info info,
> > > +                               enum adxl313_activity_type type_act,
> > > +                               int *val, int *val2)
> > > +{
> > > +     switch (info) {
> > > +     case IIO_EV_INFO_VALUE:
> > > +             return _adxl313_read_mag_value(data, dir,  
> >
> > Same issue as below for read functions.
> >  
> > > +                                            type_act,
> > > +                                            val, val2);
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static int adxl313_write_mag_value(struct adxl313_data *data,  
> >
> > This has me a little confused. It's called
> > adxl313_write_mag_value() which seems pretty specific but
> > then calls another level of _adxl313_write_mag_value.
> >
> > In the next patch (assuming diff isn't leading me astray) we have
> >
> > @@ -600,13 +687,17 @@ static int adxl313_write_mag_value(struct adxl313_data *data,
> >                                    enum iio_event_direction dir,
> >                                    enum iio_event_info info,
> >                                    enum adxl313_activity_type type_act,
> > +                                  enum adxl313_activity_type type_inact,
> >                                    int val, int val2)
> >  {
> >         switch (info) {
> >         case IIO_EV_INFO_VALUE:
> >                 return _adxl313_write_mag_value(data, dir,
> >                                                 type_act,
> > +                                               type_inact,
> >                                                 val, val2);
> > +       case IIO_EV_INFO_PERIOD:
> > +               return adxl313_set_inact_time_s(data, val);
> >         default:
> >                 return -EINVAL;
> >         }
> >
> >
> > Which is adding PERIOD to something called write_mag_value()
> >
> > Whilst I can see why you ended up with naming as:
> >
> > adxl313_write_mag_value() as the magnitude event specific part of
> > adxl13_event_write_value()
> >
> > and indeed
> >
> > _adxl313_write_mag_value() as the thing that writes IIO_EV_INFO_VALUE
> > value (i.e. the threshold) for the magnitude events.
> >
> > Last time I hit a similar naming stack, I spinkled in some _info
> >
> > So have the inner one called something slightly more specific like
> >
> > adxl313_write_mag_info_value()
> >
> >  
> > > +                                enum iio_event_direction dir,
> > > +                                enum iio_event_info info,
> > > +                                enum adxl313_activity_type type_act,
> > > +                                int val, int val2)
> > > +{
> > > +     switch (info) {
> > > +     case IIO_EV_INFO_VALUE:
> > > +             return _adxl313_write_mag_value(data, dir,
> > > +                                             type_act,
> > > +                                             val, val2);
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static int adxl313_read_event_value(struct iio_dev *indio_dev,
> > > +                                 const struct iio_chan_spec *chan,
> > > +                                 enum iio_event_type type,
> > > +                                 enum iio_event_direction dir,
> > > +                                 enum iio_event_info info,
> > > +                                 int *val, int *val2)
> > > +{
> > > +     struct adxl313_data *data = iio_priv(indio_dev);
> > > +
> > > +     switch (type) {
> > > +     case IIO_EV_TYPE_MAG:
> > > +             return adxl313_read_mag_value(data, dir, info,
> > > +                                           ADXL313_ACTIVITY,
> > > +                                           val, val2);
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +
> > > +static int adxl313_write_event_value(struct iio_dev *indio_dev,
> > > +                                  const struct iio_chan_spec *chan,
> > > +                                  enum iio_event_type type,
> > > +                                  enum iio_event_direction dir,
> > > +                                  enum iio_event_info info,
> > > +                                  int val, int val2)
> > > +{
> > > +     struct adxl313_data *data = iio_priv(indio_dev);
> > > +
> > > +     switch (type) {
> > > +     case IIO_EV_TYPE_MAG:
> > > +             return adxl313_write_mag_value(data, dir, info,
> > > +                                            ADXL313_ACTIVITY,
> > > +                                            val, val2);
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +}
> > > +  
> >
> > Otherwise LGTM
> >  
> 
> Hi, I'm about to wrap this up for the final version (let's see...).
> 
> I understand that three levels of switch/case are not good. Instead
> here I did a particular function/helper per switch/case level.
> Finally, I ended up with, e.g.
> 
> adxl313_write_event_value()  // calls
>   \-> adxl313_write_mag_value()  // calls
>          \-> _adxl313_write_mag_value()
> 
> Personally, I think, why not just having the following calls hierarchy:
> 
> adxl313_write_event_value()  // calls
>   \-> adxl313_write_mag_value()
> 
> First question: Regarding the adxl345 driver, with a little higher
> level of complexity, I adopted such a solution keeping still 2 levels
> of switch case inside the helper. Would this be ok for the ADXL313,
> too? I mean, having just one helper, but that one containing one level
> of nested switch case inside a switch/case?

I think a bit of nesting is fine but it depends on whether we end
up with conditionals etc in the inner most nest.  If that's going
on or the code is otherwise complex then breaking it up into single
layers makes sense.

> 
> 
> Another question about the naming of the helper. As you saw, I went
> "creative" and used the commonly used name for such functions
> replacing "_event_" by "_mag_". I see this can be confusing, but also
> it might make clear where the (only locally used) helper belongs to.
> 
> I understand names with leading '_' are not likely to be a decent
> choice here. But in general in case of adxl313_write_mag_value() -like
> names. What would be a better name for it, or would it be ok?

mag_value was fine, it was only when you then use the same *write_mag_value
postfix to mean the IIO_EV_INFO_VALUE of the outer *write_mag_value
that things got problematic.  Hence suggestion to use
write_mag_info_value postfix for that inner most call. For the outer calls
the write_mag_value() postfix was fine.

> 
> By the answers given to the above, and if you don't object I would
> like to prepare the single level of helper approach (then having one
> nested switch/case) and keep just the adxl313_*_mag_value() or
> ..._config() functions. Let me know what you think.

The nesting comments were from Andy (IIRC), so perhaps he can offer
some feedback on what he feels is reasonable.

Thanks,

Jonathan

> 
> > Jonathan  
>