| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAAGUq_qpe9JPp4pWR_vmo-MgWBDeWXz-+VsXHJNJ-ZtqnzB1Jw@mail.gmail.com>
Date: Mon, 13 Nov 2017 23:36:43 -0500
From: harinath Nampally <harinath922@...il.com>
To: Martin Kepplinger <martink@...teo.de>
Cc: Jonathan Cameron <jic23@...nel.org>, knaack.h@....de,
lars@...afoo.de, Peter Meerwald-Stadler <pmeerw@...erw.net>,
Greg KH <gregkh@...uxfoundation.org>,
linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
Alison Schofield <amsfield22@...il.com>
Subject: Re: [PATCH] iio: accel: mma8452: Add single pulse/tap event detection
> > This patch adds following related changes:
> > - defines pulse event related registers
> > - enables and handles single pulse interrupt for fxls8471
> > - handles IIO_EV_DIR_EITHER in read/write callbacks (because
> > event direction for pulse is either rising or falling)
> > - configures read/write event value for pulse latency register
> > using IIO_EV_INFO_HYSTERESIS
> > - adds multiple events like pulse and tranient event spec
> > as elements of event_spec array named 'mma8452_accel_events'
> >
> > Except mma8653 chip all other chips like mma845x and
> > fxls8471 have single tap detection feature.
> > Tested thoroughly using iio_event_monitor application on
> > imx6ul-evk board which has fxls8471.
> >
> > Signed-off-by: Harinath Nampally <harinath922@...il.com>
> > ---
> What tree is this written against? It doesn't apply to the current -next
> anyways.
Thanks for the review.
It is actually against 'testing' branch, I think two of my earlier
patches are not yet applied to
any branch, that might be reason this patch is not good against
current -next or 'togreg'.
> I think the defintions would deserve to be in a separate patch, but
> that's debatable.
Yes, I would argue that definitions are not a logical change.
> > .type = IIO_EV_TYPE_MAG,
> > .dir = IIO_EV_DIR_RISING,
> > .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > @@ -1139,6 +1274,15 @@ static const struct iio_event_spec mma8452_transient_event[] = {
> > BIT(IIO_EV_INFO_PERIOD) |
> > BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB)
> > },
> > + {
> > + //pulse event
> > + .type = IIO_EV_TYPE_MAG,
> > + .dir = IIO_EV_DIR_EITHER,
> > + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
> > + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
> > + BIT(IIO_EV_INFO_PERIOD) |
> > + BIT(IIO_EV_INFO_HYSTERESIS)
> > + },
> > };
> >
> > static const struct iio_event_spec mma8452_motion_event[] = {
> > @@ -1202,8 +1346,8 @@ static struct attribute_group mma8452_event_attribute_group = {
> > .shift = 16 - (bits), \
> > .endianness = IIO_BE, \
> > }, \
> > - .event_spec = mma8452_transient_event, \
> > - .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
> > + .event_spec = mma8452_accel_events, \
> > + .num_event_specs = ARRAY_SIZE(mma8452_accel_events), \
> that would go in the mentioned separate renaming-patch
OK so I will make a patch set; patch 1/2 to just rename
'mma8452_transient_event[]'
to 'mma8452_accel_events[]'(without adding pulse event).
and everything else would go in 2/2. Does that makes sense?
Thanks,
Harinath
On Fri, Nov 10, 2017 at 5:35 PM, Martin Kepplinger <martink@...teo.de> wrote:
> On 2017-11-09 04:12, Harinath Nampally wrote:
>> This patch adds following related changes:
>> - defines pulse event related registers
>> - enables and handles single pulse interrupt for fxls8471
>> - handles IIO_EV_DIR_EITHER in read/write callbacks (because
>> event direction for pulse is either rising or falling)
>> - configures read/write event value for pulse latency register
>> using IIO_EV_INFO_HYSTERESIS
>> - adds multiple events like pulse and tranient event spec
>> as elements of event_spec array named 'mma8452_accel_events'
>>
>> Except mma8653 chip all other chips like mma845x and
>> fxls8471 have single tap detection feature.
>> Tested thoroughly using iio_event_monitor application on
>> imx6ul-evk board which has fxls8471.
>>
>> Signed-off-by: Harinath Nampally <harinath922@...il.com>
>> ---
>
> What tree is this written against? It doesn't apply to the current -next
> anyways.
>
>> drivers/iio/accel/mma8452.c | 156 ++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 151 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
>> index 43c3a6b..36f1b56 100644
>> --- a/drivers/iio/accel/mma8452.c
>> +++ b/drivers/iio/accel/mma8452.c
>> @@ -72,6 +72,19 @@
>> #define MMA8452_TRANSIENT_THS_MASK GENMASK(6, 0)
>> #define MMA8452_TRANSIENT_COUNT 0x20
>> #define MMA8452_TRANSIENT_CHAN_SHIFT 1
>> +#define MMA8452_PULSE_CFG 0x21
>> +#define MMA8452_PULSE_CFG_CHAN(chan) BIT(chan * 2)
>> +#define MMA8452_PULSE_CFG_ELE BIT(6)
>> +#define MMA8452_PULSE_SRC 0x22
>> +#define MMA8452_PULSE_SRC_XPULSE BIT(4)
>> +#define MMA8452_PULSE_SRC_YPULSE BIT(5)
>> +#define MMA8452_PULSE_SRC_ZPULSE BIT(6)
>> +#define MMA8452_PULSE_THS 0x23
>> +#define MMA8452_PULSE_THS_MASK GENMASK(6, 0)
>> +#define MMA8452_PULSE_COUNT 0x26
>> +#define MMA8452_PULSE_CHAN_SHIFT 2
>> +#define MMA8452_PULSE_LTCY 0x27
>> +
>> #define MMA8452_CTRL_REG1 0x2a
>> #define MMA8452_CTRL_ACTIVE BIT(0)
>> #define MMA8452_CTRL_DR_MASK GENMASK(5, 3)
>> @@ -91,6 +104,7 @@
>>
>> #define MMA8452_INT_DRDY BIT(0)
>> #define MMA8452_INT_FF_MT BIT(2)
>> +#define MMA8452_INT_PULSE BIT(3)
>> #define MMA8452_INT_TRANS BIT(5)
>>
> I think the defintions would deserve to be in a separate patch, but
> that's debatable.
>
>> #define MMA8451_DEVICE_ID 0x1a
>> @@ -155,6 +169,16 @@ static const struct mma8452_event_regs trans_ev_regs = {
>> .ev_count = MMA8452_TRANSIENT_COUNT,
>> };
>>
>> +static const struct mma8452_event_regs pulse_ev_regs = {
>> + .ev_cfg = MMA8452_PULSE_CFG,
>> + .ev_cfg_ele = MMA8452_PULSE_CFG_ELE,
>> + .ev_cfg_chan_shift = MMA8452_PULSE_CHAN_SHIFT,
>> + .ev_src = MMA8452_PULSE_SRC,
>> + .ev_ths = MMA8452_PULSE_THS,
>> + .ev_ths_mask = MMA8452_PULSE_THS_MASK,
>> + .ev_count = MMA8452_PULSE_COUNT,
>> +};
>> +
>> /**
>> * struct mma_chip_info - chip specific data
>> * @chip_id: WHO_AM_I register's value
>> @@ -784,6 +808,14 @@ static int mma8452_get_event_regs(struct mma8452_data *data,
>> case IIO_EV_DIR_FALLING:
>> *ev_reg = &ff_mt_ev_regs;
>> return 0;
>> + case IIO_EV_DIR_EITHER:
>> + if (!(data->chip_info->all_events
>> + & MMA8452_INT_PULSE) ||
>> + !(data->chip_info->enabled_events
>> + & MMA8452_INT_PULSE))
>> + return -EINVAL;
>> + *ev_reg = &pulse_ev_regs;
>> + return 0;
>> default:
>> return -EINVAL;
>> }
>> @@ -848,6 +880,25 @@ static int mma8452_read_event_value(struct iio_dev *indio_dev,
>> return ret;
>> }
>>
>> + case IIO_EV_INFO_HYSTERESIS:
>> + if (!(data->chip_info->all_events & MMA8452_INT_PULSE) ||
>> + !(data->chip_info->enabled_events & MMA8452_INT_PULSE))
>> + return -EINVAL;
>> +
>> + ret = i2c_smbus_read_byte_data(data->client,
>> + MMA8452_PULSE_LTCY);
>> + if (ret < 0)
>> + return ret;
>> +
>> + power_mode = mma8452_get_power_mode(data);
>> + if (power_mode < 0)
>> + return power_mode;
>> +
>> + us = ret * mma8452_time_step_us[power_mode][
>> + mma8452_get_odr_index(data)];
>> + *val = us / USEC_PER_SEC;
>> + *val2 = us % USEC_PER_SEC;
>> +
>> return IIO_VAL_INT_PLUS_MICRO;
>>
>> default:
>> @@ -908,6 +959,24 @@ static int mma8452_write_event_value(struct iio_dev *indio_dev,
>>
>> return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, reg);
>>
>> + case IIO_EV_INFO_HYSTERESIS:
>> + if (!(data->chip_info->all_events & MMA8452_INT_PULSE) ||
>> + !(data->chip_info->enabled_events & MMA8452_INT_PULSE))
>> + return -EINVAL;
>> +
>> + ret = mma8452_get_power_mode(data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + steps = (val * USEC_PER_SEC + val2) /
>> + mma8452_time_step_us[ret][
>> + mma8452_get_odr_index(data)];
>> +
>> + if (steps < 0 || steps > 0xff)
>> + return -EINVAL;
>> +
>> + return mma8452_change_config(data, MMA8452_PULSE_LTCY, steps);
>> +
>> default:
>> return -EINVAL;
>> }
>> @@ -937,6 +1006,14 @@ static int mma8452_read_event_config(struct iio_dev *indio_dev,
>>
>> return !!(ret & BIT(chan->scan_index +
>> ev_regs->ev_cfg_chan_shift));
>> + case IIO_EV_DIR_EITHER:
>> + ret = i2c_smbus_read_byte_data(data->client,
>> + ev_regs->ev_cfg);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return !!(ret & BIT(chan->scan_index *
>> + ev_regs->ev_cfg_chan_shift));
>> default:
>> return -EINVAL;
>> }
>> @@ -988,6 +1065,25 @@ static int mma8452_write_event_config(struct iio_dev *indio_dev,
>> val |= ev_regs->ev_cfg_ele;
>>
>> return mma8452_change_config(data, ev_regs->ev_cfg, val);
>> +
>> + case IIO_EV_DIR_EITHER:
>> + val = i2c_smbus_read_byte_data(data->client, ev_regs->ev_cfg);
>> + if (val < 0)
>> + return val;
>> +
>> + if (state) {
>> + val &= ~BIT(chan->scan_index *
>> + ev_regs->ev_cfg_chan_shift);
>> + val |= BIT(chan->scan_index *
>> + ev_regs->ev_cfg_chan_shift);
>> + } else {
>> + val &= ~BIT(chan->scan_index *
>> + ev_regs->ev_cfg_chan_shift);
>> + }
>> +
>> + val |= ev_regs->ev_cfg_ele;
>> +
>> + return mma8452_change_config(data, ev_regs->ev_cfg, val);
>> default:
>> return -EINVAL;
>> }
>> @@ -1025,6 +1121,38 @@ static void mma8452_transient_interrupt(struct iio_dev *indio_dev)
>> ts);
>> }
>>
>> +static void mma8452_pulse_interrupt(struct iio_dev *indio_dev)
>> +{
>> + struct mma8452_data *data = iio_priv(indio_dev);
>> + s64 ts = iio_get_time_ns(indio_dev);
>> + int src;
>> +
>> + src = i2c_smbus_read_byte_data(data->client, MMA8452_PULSE_SRC);
>> + if (src < 0)
>> + return;
>> +
>> + if (src & MMA8452_PULSE_SRC_XPULSE)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
>> + IIO_EV_TYPE_MAG,
>> + IIO_EV_DIR_EITHER),
>> + ts);
>> +
>> + if (src & MMA8452_PULSE_SRC_YPULSE)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Y,
>> + IIO_EV_TYPE_MAG,
>> + IIO_EV_DIR_EITHER),
>> + ts);
>> +
>> + if (src & MMA8452_PULSE_SRC_ZPULSE)
>> + iio_push_event(indio_dev,
>> + IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Z,
>> + IIO_EV_TYPE_MAG,
>> + IIO_EV_DIR_EITHER),
>> + ts);
>> +}
>> +
>> static irqreturn_t mma8452_interrupt(int irq, void *p)
>> {
>> struct iio_dev *indio_dev = p;
>> @@ -1063,6 +1191,11 @@ static irqreturn_t mma8452_interrupt(int irq, void *p)
>> ret = IRQ_HANDLED;
>> }
>>
>> + if (src & MMA8452_INT_PULSE) {
>> + mma8452_pulse_interrupt(indio_dev);
>> + ret = IRQ_HANDLED;
>> + }
>> +
>> return ret;
>> }
>>
>> @@ -1130,8 +1263,10 @@ static const struct iio_event_spec mma8652_freefall_event[] = {
>> },
>> };
>>
>> -static const struct iio_event_spec mma8452_transient_event[] = {
>> +
>> +static const struct iio_event_spec mma8452_accel_events[] = {
>
> I would prefer having this renaming in a separate patch too - with a
> good expanation about why. (Don't be afraid of longer commit messages)
>
>> {
>> + //trasient event
>
> typo?
>
>> .type = IIO_EV_TYPE_MAG,
>> .dir = IIO_EV_DIR_RISING,
>> .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>> @@ -1139,6 +1274,15 @@ static const struct iio_event_spec mma8452_transient_event[] = {
>> BIT(IIO_EV_INFO_PERIOD) |
>> BIT(IIO_EV_INFO_HIGH_PASS_FILTER_3DB)
>> },
>> + {
>> + //pulse event
>> + .type = IIO_EV_TYPE_MAG,
>> + .dir = IIO_EV_DIR_EITHER,
>> + .mask_separate = BIT(IIO_EV_INFO_ENABLE),
>> + .mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_PERIOD) |
>> + BIT(IIO_EV_INFO_HYSTERESIS)
>> + },
>> };
>>
>> static const struct iio_event_spec mma8452_motion_event[] = {
>> @@ -1202,8 +1346,8 @@ static struct attribute_group mma8452_event_attribute_group = {
>> .shift = 16 - (bits), \
>> .endianness = IIO_BE, \
>> }, \
>> - .event_spec = mma8452_transient_event, \
>> - .num_event_specs = ARRAY_SIZE(mma8452_transient_event), \
>> + .event_spec = mma8452_accel_events, \
>> + .num_event_specs = ARRAY_SIZE(mma8452_accel_events), \
>
> that would go in the mentioned separate renaming-patch
>
>> }
>>
>> #define MMA8652_CHANNEL(axis, idx, bits) { \
>> @@ -1368,9 +1512,11 @@ static const struct mma_chip_info mma_chip_info_table[] = {
>> */
>> .all_events = MMA8452_INT_DRDY |
>> MMA8452_INT_TRANS |
>> - MMA8452_INT_FF_MT,
>> + MMA8452_INT_FF_MT |
>> + MMA8452_INT_PULSE,
>> .enabled_events = MMA8452_INT_TRANS |
>> - MMA8452_INT_FF_MT,
>> + MMA8452_INT_FF_MT |
>> + MMA8452_INT_PULSE,
>> },
>> };
>>
>>
>
Powered by blists - more mailing lists