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

[harinath Nampally]

> I stripped the version change stuff from the commit message - they
> should have been below the ---  Useful during review, but generally
> not worth retaining once we have accepted it.

I didn't know that, thanks for letting me know.
Next time I will keep that in mind.

Thanks,
Hari

On Sun, Sep 10, 2017 at 11:51 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Sun, 10 Sep 2017 08:36:43 +0200
> 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>
>>
> Applied to the togreg branch of iio.git - pushed out as testing to
> let the autobuilders play with it.
>
> I stripped the version change stuff from the commit message - they
> should have been below the ---  Useful during review, but generally
> not worth retaining once we have accepted it.
>
> Thanks,
>
> Jonathan
>>
>> 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;
>> > +   }
>> > +}
>> > +
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>