Re: [PATCH v6] iio: accel: mma8452: improvements to handle multiple events

[harinath Nampally]

> Alright, I didn't test it but I kind of like it now. The one minor
> naming issue I had pointed out before is mentioned below. But if that's
> no issue for Jon:
> Reviewed-by: Martin Kepplinger <martink@xxxxxxxxx>
Martin, Thanks a lot for the review.

> btw, Harianath: Would you point me to the imx6ul eval board you are
> using? thanks
https://www.nxp.com/products/microcontrollers-and-processors/arm-based-processors-and-mcus/i.mx-applications-processors/developer-resources/i.mx6ultralite-evaluation-kit:MCIMX6UL-EVK

> > @@ -740,6 +762,36 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
> >       return ret;
> >  }
> >
> > +static int mma8452_get_event_regs(struct mma8452_data *data,
> > +             const struct iio_chan_spec *chan, enum iio_event_direction dir,
> > +             const struct mma8452_event_regs **ev_reg)
> > +{
> > +     if (!chan)
> > +             return -EINVAL;
> > +
> > +     switch (chan->type) {
> > +     case IIO_ACCEL:
> > +             switch (dir) {
> > +             case IIO_EV_DIR_RISING:
> > +                     if ((data->chip_info->all_events
> > +                                     & MMA8452_INT_TRANS) &&
> > +                             (data->chip_info->enabled_events
> > +                                     & MMA8452_INT_TRANS))
> > +                             *ev_reg = &ev_regs_accel_rising;
> > +                     else
> > +                             *ev_reg = &ev_regs_accel_falling;
> > +                     return 0;
> I know it's fine, only the naming seems odd here.
I agree, I will replace 'ev_regs_accel_rising' to 'ev_trans_regs'
and 'ev_regs_accel_falling' to 'ev_ff_mt_regs'.
As Jon just applied this patch, I will cover this in my next patch set
which fix the checkpatch.pl warnings in this file.

Thanks,
Hari


On Sun, Sep 10, 2017 at 2:36 AM, Martin Kepplinger <martink@xxxxxxxxx> wrote:
>
> On 2017-09-09 21:56, Harinath Nampally wrote:
> >     This driver supports multiple devices like mma8653,
> >     mma8652, mma8452, mma8453 and fxls8471. Almost all
> >     these devices have more than one event.
> >
> >     Current driver design hardcodes the event specific
> >     information, so only one event can be supported by this
> >     driver at any given time.
> >     Also current design doesn't have the flexibility to
> >     add more events.
> >
> >     This patch improves by detaching the event related
> >     information from chip_info struct,and based on channel
> >     type and event direction the corresponding event
> >     configuration registers are picked dynamically.
> >     Hence both transient and freefall events can be
> >     handled in read/write callbacks.
> >
> >     Changes are thoroughly tested on fxls8471 device on imx6UL
> >     Eval board using iio_event_monitor user space program.
> >
> >     After this fix both Freefall and Transient events are
> >     handled by the driver without any conflicts.
> >
> >     Changes since v5 -> v6
> >     -Rename supported_events to all_events(short name)
> >     -Use get_event_regs function in read/write event
> >      config callbacks to pick appropriate config registers
> >     -Add ev_cfg_ele and ev_cfg_chan_shift in event_regs struct
> >      which are needed in read/write event callbacks
> >
> >     Changes since v4 -> v5
> >     -Add supported_events and enabled_events
> >      in chip_info structure so that devices(mma865x)
> >      which has no support for transient event will
> >      fallback to freefall event. Hence this patch changes
> >      won't break for devices that can't support
> >      transient events
> >
> >     Changes since v3 -> v4
> >     -Add 'const struct ev_regs_accel_falling'
> >     -Add 'const struct ev_regs_accel_rising'
> >     -Refactor mma8452_get_event_regs function to
> >      remove the fill in the struct and return above structs
> >     -Condense the commit's subject message
> >
> >     Changes since v2 -> v3
> >     -Fix typo in commit message
> >     -Replace word 'Bugfix' with 'Improvements'
> >     -Describe more accurate commit message
> >     -Replace breaks with returns
> >     -Initialise transient event threshold mask
> >     -Remove unrelated change of IIO_ACCEL channel type
> >      check in read/write event callbacks
> >
> >     Changes since v1 -> v2
> >     -Fix indentations
> >     -Remove unused fields in mma8452_event_regs struct
> >     -Remove redundant return statement
> >     -Remove unrelated changes like checkpatch.pl warning fixes
> >
> > Signed-off-by: Harinath Nampally <harinath922@xxxxxxxxx>
> > ---
>
> Alright, I didn't test it but I kind of like it now. The one minor
> naming issue I had pointed out before is mentioned below. But if that's
> no issue for Jon:
>
> Reviewed-by: Martin Kepplinger <martink@xxxxxxxxx>
>
>
> btw, Harianath: Would you point me to the imx6ul eval board you are
> using? thanks
>
> > @@ -740,6 +762,36 @@ static int mma8452_write_raw(struct iio_dev *indio_dev,
> >       return ret;
> >  }
> >
> > +static int mma8452_get_event_regs(struct mma8452_data *data,
> > +             const struct iio_chan_spec *chan, enum iio_event_direction dir,
> > +             const struct mma8452_event_regs **ev_reg)
> > +{
> > +     if (!chan)
> > +             return -EINVAL;
> > +
> > +     switch (chan->type) {
> > +     case IIO_ACCEL:
> > +             switch (dir) {
> > +             case IIO_EV_DIR_RISING:
> > +                     if ((data->chip_info->all_events
> > +                                     & MMA8452_INT_TRANS) &&
> > +                             (data->chip_info->enabled_events
> > +                                     & MMA8452_INT_TRANS))
> > +                             *ev_reg = &ev_regs_accel_rising;
> > +                     else
> > +                             *ev_reg = &ev_regs_accel_falling;
> > +                     return 0;
>
> I know it's fine, only the naming seems odd here.
>
> > +             case IIO_EV_DIR_FALLING:
> > +                     *ev_reg = &ev_regs_accel_falling;
> > +                     return 0;
> > +             default:
> > +                     return -EINVAL;
> > +             }
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
> > +