Re: [PATCH] iio: accel: mma8452: Bugfix to enbale and allow different events to work parallely.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Am 17. August 2017 02:39:05 MESZ schrieb Harinath Nampally <harinath922@xxxxxxxxx>:
>> Hello,
>>
>> fixes are always welcome, some comments regarding the patch
>Thanks for the review.
>>
>> in subject: typo "enable"
>Will fix it.
>>> 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 and current design doesn't have the flexibility to add more
>events.
>>   
>>> This patch fixes 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 multiple events can be handled in
>read/write callbacks.
>> which chip can have which event(s)?
>I am planning to add 'supported events' field in
>
>struct mma_chip_info which indicates which chip can have which events.
>During initialization in 'mma_chip_info_table' would set this
>'supported events' field for each chip.
>But I wonder should I add those changes as part of this patch?

is it necessary or can it be documentation?

And this patch should have been called "v2". please include a persistent version history to v3 of this patch.

thanks

>
>>   
>>> 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.
>> thanks, p.
>>   
>>> Signed-off-by: Harinath Nampally <harinath922@xxxxxxxxx>
>>> ---
>>>   drivers/iio/accel/mma8452.c | 309
>++++++++++++++++++++------------------------
>>>   1 file changed, 141 insertions(+), 168 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/mma8452.c
>b/drivers/iio/accel/mma8452.c
>>> index eb6e3dc..1b83e52 100644
>>> --- a/drivers/iio/accel/mma8452.c
>>> +++ b/drivers/iio/accel/mma8452.c
>>> @@ -59,7 +59,9 @@
>>>   #define MMA8452_FF_MT_THS			0x17
>>>   #define  MMA8452_FF_MT_THS_MASK			0x7f
>>>   #define MMA8452_FF_MT_COUNT			0x18
>>> +#define MMA8452_FF_MT_CHAN_SHIFT	3
>>>   #define MMA8452_TRANSIENT_CFG			0x1d
>>> +#define  MMA8452_TRANSIENT_CFG_CHAN(chan)	BIT(chan + 1)
>>>   #define  MMA8452_TRANSIENT_CFG_HPF_BYP		BIT(0)
>>>   #define  MMA8452_TRANSIENT_CFG_ELE		BIT(4)
>>>   #define MMA8452_TRANSIENT_SRC			0x1e
>>> @@ -69,6 +71,7 @@
>>>   #define MMA8452_TRANSIENT_THS			0x1f
>>>   #define  MMA8452_TRANSIENT_THS_MASK		GENMASK(6, 0)
>>>   #define MMA8452_TRANSIENT_COUNT			0x20
>>> +#define MMA8452_TRANSIENT_CHAN_SHIFT 1
>>>   #define MMA8452_CTRL_REG1			0x2a
>>>   #define  MMA8452_CTRL_ACTIVE			BIT(0)
>>>   #define  MMA8452_CTRL_DR_MASK			GENMASK(5, 3)
>>> @@ -107,6 +110,26 @@ struct mma8452_data {
>>>   	const struct mma_chip_info *chip_info;
>>>   };
>>>   
>>> + /**
>>> +  * struct mma8452_event_regs - chip specific data related to
>events
>>> +  * @ev_cfg:			event config register address
>>> +  * @ev_src:			event source register address
>>> +  * @ev_ths:			event threshold register address
>>> +  * @ev_ths_mask:		mask for the threshold value
>>> +  * @ev_count:			event count (period) register address
>>> +  *
>>> +  * Since not all chips supported by the driver support comparing
>high pass
>>> +  * filtered data for events (interrupts), different interrupt
>sources are
>>> +  * used for different chips and the relevant registers are
>included here.
>>> +  */
>>> +struct mma8452_event_regs {
>>> +		u8 ev_cfg;
>>> +		u8 ev_src;
>>> +		u8 ev_ths;
>>> +		u8 ev_ths_mask;
>>> +		u8 ev_count;
>>> +};
>>> +
>>>   /**
>>>    * struct mma_chip_info - chip specific data
>>>    * @chip_id:			WHO_AM_I register's value
>>> @@ -116,40 +139,12 @@ struct mma8452_data {
>>>    * @mma_scales:			scale factors for converting register values
>>>    *				to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
>>>    *				per mode: m/s^2 and micro m/s^2
>>> - * @ev_cfg:			event config register address
>>> - * @ev_cfg_ele:			latch bit in event config register
>>> - * @ev_cfg_chan_shift:		number of the bit to enable events in X
>>> - *				direction; in event config register
>>> - * @ev_src:			event source register address
>>> - * @ev_src_xe:			bit in event source register that indicates
>>> - *				an event in X direction
>>> - * @ev_src_ye:			bit in event source register that indicates
>>> - *				an event in Y direction
>>> - * @ev_src_ze:			bit in event source register that indicates
>>> - *				an event in Z direction
>>> - * @ev_ths:			event threshold register address
>>> - * @ev_ths_mask:		mask for the threshold value
>>> - * @ev_count:			event count (period) register address
>>> - *
>>> - * Since not all chips supported by the driver support comparing
>high pass
>>> - * filtered data for events (interrupts), different interrupt
>sources are
>>> - * used for different chips and the relevant registers are included
>here.
>>>    */
>>>   struct mma_chip_info {
>>>   	u8 chip_id;
>>>   	const struct iio_chan_spec *channels;
>>>   	int num_channels;
>>>   	const int mma_scales[3][2];
>>> -	u8 ev_cfg;
>>> -	u8 ev_cfg_ele;
>>> -	u8 ev_cfg_chan_shift;
>>> -	u8 ev_src;
>>> -	u8 ev_src_xe;
>>> -	u8 ev_src_ye;
>>> -	u8 ev_src_ze;
>>> -	u8 ev_ths;
>>> -	u8 ev_ths_mask;
>>> -	u8 ev_count;
>>>   };
>>>   
>>>   enum {
>>> @@ -602,9 +597,8 @@ static int mma8452_set_power_mode(struct
>mma8452_data *data, u8 mode)
>>>   static int mma8452_freefall_mode_enabled(struct mma8452_data
>*data)
>>>   {
>>>   	int val;
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   
>>> -	val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>>> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
>>>   	if (val < 0)
>>>   		return val;
>>>   
>>> @@ -614,29 +608,28 @@ static int
>mma8452_freefall_mode_enabled(struct mma8452_data *data)
>>>   static int mma8452_set_freefall_mode(struct mma8452_data *data,
>bool state)
>>>   {
>>>   	int val;
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   
>>>   	if ((state && mma8452_freefall_mode_enabled(data)) ||
>>>   	    (!state && !(mma8452_freefall_mode_enabled(data))))
>>>   		return 0;
>>>   
>>> -	val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>>> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
>>>   	if (val < 0)
>>>   		return val;
>>>   
>>>   	if (state) {
>>> -		val |= BIT(idx_x + chip->ev_cfg_chan_shift);
>>> -		val |= BIT(idx_y + chip->ev_cfg_chan_shift);
>>> -		val |= BIT(idx_z + chip->ev_cfg_chan_shift);
>>> +		val |= BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val |= BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val |= BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
>>>   		val &= ~MMA8452_FF_MT_CFG_OAE;
>>>   	} else {
>>> -		val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
>>> -		val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
>>> -		val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
>>> +		val &= ~BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val &= ~BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val &= ~BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
>>>   		val |= MMA8452_FF_MT_CFG_OAE;
>>>   	}
>>>   
>>> -	return mma8452_change_config(data, chip->ev_cfg, val);
>>> +	return mma8452_change_config(data, MMA8452_FF_MT_CFG, val);
>>>   }
>>>   
>>>   static int mma8452_set_hp_filter_frequency(struct mma8452_data
>*data,
>>> @@ -740,6 +733,39 @@ static int mma8452_write_raw(struct iio_dev
>*indio_dev,
>>>   	return ret;
>>>   }
>>>   
>>> +static int mma8452_get_event_regs(const struct iio_chan_spec *chan,
>>> +	       enum iio_event_direction dir,
>>> +		   struct mma8452_event_regs *ev_regs
>>> +	       )
>>> +{
>>> +	if (!chan)
>>> +		return -EINVAL;
>>> +	switch (chan->type) {
>>> +	case IIO_ACCEL:
>>> +		switch (dir) {
>>> +		case IIO_EV_DIR_FALLING:
>>> +			ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
>>> +			ev_regs->ev_src = MMA8452_FF_MT_SRC;
>>> +			ev_regs->ev_ths = MMA8452_FF_MT_THS;
>>> +			ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
>>> +			ev_regs->ev_count = MMA8452_FF_MT_COUNT;
>>> +			break;
>>> +		case IIO_EV_DIR_RISING:
>>> +			ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
>>> +			ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
>>> +			ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
>> ev_ths_mask is not set here
>>
>Good catch! Will fix it.
>>> +			ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
>>> +			break;
>>> +		default:
>>> +			return -EINVAL;
>>> +		}
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +	return 0;
>> could replace 'breaks' with return 0 to save some lines and simplify
>> control flow
>Sure.
>>> +}
>>> +
>>>   static int mma8452_read_thresh(struct iio_dev *indio_dev,
>>>   			       const struct iio_chan_spec *chan,
>>>   			       enum iio_event_type type,
>>> @@ -749,21 +775,23 @@ static int mma8452_read_thresh(struct iio_dev
>*indio_dev,
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>>   	int ret, us, power_mode;
>>> +	struct mma8452_event_regs ev_regs;
>>>   
>>> +	ret = mma8452_get_event_regs(chan, dir, &ev_regs);
>>> +	if (ret)
>>> +		return ret;
>>>   	switch (info) {
>>>   	case IIO_EV_INFO_VALUE:
>>> -		ret = i2c_smbus_read_byte_data(data->client,
>>> -					       data->chip_info->ev_ths);
>>> +		ret = i2c_smbus_read_byte_data(data->client, ev_regs.ev_ths);
>>>   		if (ret < 0)
>>>   			return ret;
>>>   
>>> -		*val = ret & data->chip_info->ev_ths_mask;
>>> +		*val = ret & ev_regs.ev_ths_mask;
>>>   
>>>   		return IIO_VAL_INT;
>>>   
>>>   	case IIO_EV_INFO_PERIOD:
>>> -		ret = i2c_smbus_read_byte_data(data->client,
>>> -					       data->chip_info->ev_count);
>>> +		ret = i2c_smbus_read_byte_data(data->client, ev_regs.ev_count);
>>>   		if (ret < 0)
>>>   			return ret;
>>>   
>>> @@ -809,14 +837,18 @@ static int mma8452_write_thresh(struct iio_dev
>*indio_dev,
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>>   	int ret, reg, steps;
>>> +	struct mma8452_event_regs ev_regs;
>>> +
>>> +	ret = mma8452_get_event_regs(chan, dir, &ev_regs);
>>> +	if (ret)
>>> +		return ret;
>>>   
>>>   	switch (info) {
>>>   	case IIO_EV_INFO_VALUE:
>>> -		if (val < 0 || val > MMA8452_TRANSIENT_THS_MASK)
>>> +		if (val < 0 || val > ev_regs.ev_ths_mask)
>>>   			return -EINVAL;
>>>   
>>> -		return mma8452_change_config(data, data->chip_info->ev_ths,
>>> -					     val);
>>> +		return mma8452_change_config(data, ev_regs.ev_ths, val);
>>>   
>>>   	case IIO_EV_INFO_PERIOD:
>>>   		ret = mma8452_get_power_mode(data);
>>> @@ -830,8 +862,7 @@ static int mma8452_write_thresh(struct iio_dev
>*indio_dev,
>>>   		if (steps < 0 || steps > 0xff)
>>>   			return -EINVAL;
>>>   
>>> -		return mma8452_change_config(data, data->chip_info->ev_count,
>>> -					     steps);
>>> +		return mma8452_change_config(data, ev_regs.ev_count, steps);
>>>   
>>>   	case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
>>>   		reg = i2c_smbus_read_byte_data(data->client,
>>> @@ -861,23 +892,23 @@ static int mma8452_read_event_config(struct
>iio_dev *indio_dev,
>>>   				     enum iio_event_direction dir)
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   	int ret;
>>>   
>>> -	switch (dir) {
>>> -	case IIO_EV_DIR_FALLING:
>>> -		return mma8452_freefall_mode_enabled(data);
>>> -	case IIO_EV_DIR_RISING:
>>> -		if (mma8452_freefall_mode_enabled(data))
>>> -			return 0;
>>> +	switch (chan->type) {
>>> +	case IIO_ACCEL:
>> this adds a check for chan-type == IIO_ACCESS, so strictly this would
>be
>> an unrelated change...
>Ok will remove it from this patch.
>>> +		switch (dir) {
>>> +		case IIO_EV_DIR_FALLING:
>>> +			return mma8452_freefall_mode_enabled(data);
>>> +		case IIO_EV_DIR_RISING:
>>> +			ret = i2c_smbus_read_byte_data(data->client,
>MMA8452_TRANSIENT_CFG);
>>> +			if (ret < 0)
>>> +				return ret;
>>>   
>>> -		ret = i2c_smbus_read_byte_data(data->client,
>>> -					       data->chip_info->ev_cfg);
>>> -		if (ret < 0)
>>> -			return ret;
>>> +			return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
>>>   
>>> -		return !!(ret & BIT(chan->scan_index +
>>> -				    chip->ev_cfg_chan_shift));
>>> +		default:
>>> +			return -EINVAL;
>>> +		}
>>>   	default:
>>>   		return -EINVAL;
>>>   	}
>>> @@ -890,39 +921,33 @@ static int mma8452_write_event_config(struct
>iio_dev *indio_dev,
>>>   				      int state)
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   	int val, ret;
>>>   
>>>   	ret = mma8452_set_runtime_pm_state(data->client, state);
>>>   	if (ret)
>>>   		return ret;
>>>   
>>> -	switch (dir) {
>>> -	case IIO_EV_DIR_FALLING:
>>> -		return mma8452_set_freefall_mode(data, state);
>>> -	case IIO_EV_DIR_RISING:
>>> -		val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>>> -		if (val < 0)
>>> -			return val;
>>> -
>>> -		if (state) {
>>> -			if (mma8452_freefall_mode_enabled(data)) {
>>> -				val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
>>> -				val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
>>> -				val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
>>> -				val |= MMA8452_FF_MT_CFG_OAE;
>>> -			}
>>> -			val |= BIT(chan->scan_index + chip->ev_cfg_chan_shift);
>>> -		} else {
>>> -			if (mma8452_freefall_mode_enabled(data))
>>> -				return 0;
>>> -
>>> -			val &= ~BIT(chan->scan_index + chip->ev_cfg_chan_shift);
>>> +	switch (chan->type) {
>>> +	case IIO_ACCEL:
>>> +		switch (dir) {
>>> +		case IIO_EV_DIR_FALLING:
>>> +			return mma8452_set_freefall_mode(data, state);
>>> +		case IIO_EV_DIR_RISING:
>>> +			val = i2c_smbus_read_byte_data(data->client,
>MMA8452_TRANSIENT_CFG);
>>> +			if (val < 0)
>>> +				return val;
>>> +
>>> +			if (state)
>>> +				val |= MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>>> +			else
>>> +				val &= ~MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index);
>>> +
>>> +			val |= MMA8452_TRANSIENT_CFG_ELE;
>>> +
>>> +			return mma8452_change_config(data, MMA8452_TRANSIENT_CFG, val);
>>> +		default:
>>> +			return -EINVAL;
>>>   		}
>>> -
>>> -		val |= chip->ev_cfg_ele;
>>> -
>>> -		return mma8452_change_config(data, chip->ev_cfg, val);
>>>   	default:
>>>   		return -EINVAL;
>>>   	}
>>> @@ -934,35 +959,25 @@ static void mma8452_transient_interrupt(struct
>iio_dev *indio_dev)
>>>   	s64 ts = iio_get_time_ns(indio_dev);
>>>   	int src;
>>>   
>>> -	src = i2c_smbus_read_byte_data(data->client,
>data->chip_info->ev_src);
>>> +	src = i2c_smbus_read_byte_data(data->client,
>MMA8452_TRANSIENT_SRC);
>>>   	if (src < 0)
>>>   		return;
>>>   
>>> -	if (mma8452_freefall_mode_enabled(data)) {
>>> -		iio_push_event(indio_dev,
>>> -			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
>>> -						  IIO_MOD_X_AND_Y_AND_Z,
>>> -						  IIO_EV_TYPE_MAG,
>>> -						  IIO_EV_DIR_FALLING),
>>> -			       ts);
>>> -		return;
>>> -	}
>>> -
>>> -	if (src & data->chip_info->ev_src_xe)
>>> +	if (src & MMA8452_TRANSIENT_SRC_XTRANSE)
>>>   		iio_push_event(indio_dev,
>>>   			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_X,
>>>   						  IIO_EV_TYPE_MAG,
>>>   						  IIO_EV_DIR_RISING),
>>>   			       ts);
>>>   
>>> -	if (src & data->chip_info->ev_src_ye)
>>> +	if (src & MMA8452_TRANSIENT_SRC_YTRANSE)
>>>   		iio_push_event(indio_dev,
>>>   			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Y,
>>>   						  IIO_EV_TYPE_MAG,
>>>   						  IIO_EV_DIR_RISING),
>>>   			       ts);
>>>   
>>> -	if (src & data->chip_info->ev_src_ze)
>>> +	if (src & MMA8452_TRANSIENT_SRC_ZTRANSE)
>>>   		iio_push_event(indio_dev,
>>>   			       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0, IIO_MOD_Z,
>>>   						  IIO_EV_TYPE_MAG,
>>> @@ -974,23 +989,41 @@ static irqreturn_t mma8452_interrupt(int irq,
>void *p)
>>>   {
>>>   	struct iio_dev *indio_dev = p;
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   	int ret = IRQ_NONE;
>>> -	int src;
>>> +	int src, enabled_interrupts;
>>>   
>>>   	src = i2c_smbus_read_byte_data(data->client, MMA8452_INT_SRC);
>>>   	if (src < 0)
>>>   		return IRQ_NONE;
>>>   
>>> +	enabled_interrupts = i2c_smbus_read_byte_data(data->client,
>>> +					MMA8452_CTRL_REG4);
>>> +	if (enabled_interrupts < 0)
>>> +		return enabled_interrupts;
>>> +
>>> +	if (!(src & enabled_interrupts))
>>> +		return IRQ_NONE;
>>> +
>>>   	if (src & MMA8452_INT_DRDY) {
>>>   		iio_trigger_poll_chained(indio_dev->trig);
>>>   		ret = IRQ_HANDLED;
>>>   	}
>>>   
>>> -	if ((src & MMA8452_INT_TRANS &&
>>> -	     chip->ev_src == MMA8452_TRANSIENT_SRC) ||
>>> -	    (src & MMA8452_INT_FF_MT &&
>>> -	     chip->ev_src == MMA8452_FF_MT_SRC)) {
>>> +	if (src & MMA8452_INT_FF_MT) {
>>> +		if (mma8452_freefall_mode_enabled(data)) {
>>> +			s64 ts = iio_get_time_ns(indio_dev);
>>> +
>>> +			iio_push_event(indio_dev,
>>> +				       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
>>> +							  IIO_MOD_X_AND_Y_AND_Z,
>>> +							  IIO_EV_TYPE_MAG,
>>> +							  IIO_EV_DIR_FALLING),
>>> +				       ts);
>>> +		}
>>> +		ret = IRQ_HANDLED;
>>> +	}
>>> +
>>> +	if (src & MMA8452_INT_TRANS) {
>>>   		mma8452_transient_interrupt(indio_dev);
>>>   		ret = IRQ_HANDLED;
>>>   	}
>>> @@ -1222,96 +1255,36 @@ static const struct mma_chip_info
>mma_chip_info_table[] = {
>>>   		 *	g * N * 1000000 / 2048 for N = 2, 4, 8 and g=9.80665
>>>   		 */
>>>   		.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
>>> -		.ev_cfg = MMA8452_TRANSIENT_CFG,
>>> -		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
>>> -		.ev_cfg_chan_shift = 1,
>>> -		.ev_src = MMA8452_TRANSIENT_SRC,
>>> -		.ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
>>> -		.ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
>>> -		.ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
>>> -		.ev_ths = MMA8452_TRANSIENT_THS,
>>> -		.ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
>>> -		.ev_count = MMA8452_TRANSIENT_COUNT,
>>>   	},
>>>   	[mma8452] = {
>>>   		.chip_id = MMA8452_DEVICE_ID,
>>>   		.channels = mma8452_channels,
>>>   		.num_channels = ARRAY_SIZE(mma8452_channels),
>>>   		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
>>> -		.ev_cfg = MMA8452_TRANSIENT_CFG,
>>> -		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
>>> -		.ev_cfg_chan_shift = 1,
>>> -		.ev_src = MMA8452_TRANSIENT_SRC,
>>> -		.ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
>>> -		.ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
>>> -		.ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
>>> -		.ev_ths = MMA8452_TRANSIENT_THS,
>>> -		.ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
>>> -		.ev_count = MMA8452_TRANSIENT_COUNT,
>>>   	},
>>>   	[mma8453] = {
>>>   		.chip_id = MMA8453_DEVICE_ID,
>>>   		.channels = mma8453_channels,
>>>   		.num_channels = ARRAY_SIZE(mma8453_channels),
>>>   		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
>>> -		.ev_cfg = MMA8452_TRANSIENT_CFG,
>>> -		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
>>> -		.ev_cfg_chan_shift = 1,
>>> -		.ev_src = MMA8452_TRANSIENT_SRC,
>>> -		.ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
>>> -		.ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
>>> -		.ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
>>> -		.ev_ths = MMA8452_TRANSIENT_THS,
>>> -		.ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
>>> -		.ev_count = MMA8452_TRANSIENT_COUNT,
>>>   	},
>>>   	[mma8652] = {
>>>   		.chip_id = MMA8652_DEVICE_ID,
>>>   		.channels = mma8652_channels,
>>>   		.num_channels = ARRAY_SIZE(mma8652_channels),
>>>   		.mma_scales = { {0, 9577}, {0, 19154}, {0, 38307} },
>>> -		.ev_cfg = MMA8452_FF_MT_CFG,
>>> -		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
>>> -		.ev_cfg_chan_shift = 3,
>>> -		.ev_src = MMA8452_FF_MT_SRC,
>>> -		.ev_src_xe = MMA8452_FF_MT_SRC_XHE,
>>> -		.ev_src_ye = MMA8452_FF_MT_SRC_YHE,
>>> -		.ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
>>> -		.ev_ths = MMA8452_FF_MT_THS,
>>> -		.ev_ths_mask = MMA8452_FF_MT_THS_MASK,
>>> -		.ev_count = MMA8452_FF_MT_COUNT,
>>>   	},
>>>   	[mma8653] = {
>>>   		.chip_id = MMA8653_DEVICE_ID,
>>>   		.channels = mma8653_channels,
>>>   		.num_channels = ARRAY_SIZE(mma8653_channels),
>>>   		.mma_scales = { {0, 38307}, {0, 76614}, {0, 153228} },
>>> -		.ev_cfg = MMA8452_FF_MT_CFG,
>>> -		.ev_cfg_ele = MMA8452_FF_MT_CFG_ELE,
>>> -		.ev_cfg_chan_shift = 3,
>>> -		.ev_src = MMA8452_FF_MT_SRC,
>>> -		.ev_src_xe = MMA8452_FF_MT_SRC_XHE,
>>> -		.ev_src_ye = MMA8452_FF_MT_SRC_YHE,
>>> -		.ev_src_ze = MMA8452_FF_MT_SRC_ZHE,
>>> -		.ev_ths = MMA8452_FF_MT_THS,
>>> -		.ev_ths_mask = MMA8452_FF_MT_THS_MASK,
>>> -		.ev_count = MMA8452_FF_MT_COUNT,
>>>   	},
>>>   	[fxls8471] = {
>>>   		.chip_id = FXLS8471_DEVICE_ID,
>>>   		.channels = mma8451_channels,
>>>   		.num_channels = ARRAY_SIZE(mma8451_channels),
>>>   		.mma_scales = { {0, 2394}, {0, 4788}, {0, 9577} },
>>> -		.ev_cfg = MMA8452_TRANSIENT_CFG,
>>> -		.ev_cfg_ele = MMA8452_TRANSIENT_CFG_ELE,
>>> -		.ev_cfg_chan_shift = 1,
>>> -		.ev_src = MMA8452_TRANSIENT_SRC,
>>> -		.ev_src_xe = MMA8452_TRANSIENT_SRC_XTRANSE,
>>> -		.ev_src_ye = MMA8452_TRANSIENT_SRC_YTRANSE,
>>> -		.ev_src_ze = MMA8452_TRANSIENT_SRC_ZTRANSE,
>>> -		.ev_ths = MMA8452_TRANSIENT_THS,
>>> -		.ev_ths_mask = MMA8452_TRANSIENT_THS_MASK,
>>> -		.ev_count = MMA8452_TRANSIENT_COUNT,
>>>   	},
>>>   };
>>>   
>>>
>
>On 08/16/2017 09:12 AM, Peter Meerwald-Stadler wrote:
>> Hello,
>>
>> fixes are always welcome, some comments regarding the patch
>>
>> in subject: typo "enable"
>>
>>> 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 and current design doesn't have the flexibility to add more
>events.
>>   
>>> This patch fixes 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 multiple events can be handled in
>read/write callbacks.
>> which chip can have which event(s)?
>>   
>>> 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.
>> thanks, p.
>>   
>>> Signed-off-by: Harinath Nampally <harinath922@xxxxxxxxx>
>>> ---
>>>   drivers/iio/accel/mma8452.c | 309
>++++++++++++++++++++------------------------
>>>   1 file changed, 141 insertions(+), 168 deletions(-)
>>>
>>> diff --git a/drivers/iio/accel/mma8452.c
>b/drivers/iio/accel/mma8452.c
>>> index eb6e3dc..1b83e52 100644
>>> --- a/drivers/iio/accel/mma8452.c
>>> +++ b/drivers/iio/accel/mma8452.c
>>> @@ -59,7 +59,9 @@
>>>   #define MMA8452_FF_MT_THS			0x17
>>>   #define  MMA8452_FF_MT_THS_MASK			0x7f
>>>   #define MMA8452_FF_MT_COUNT			0x18
>>> +#define MMA8452_FF_MT_CHAN_SHIFT	3
>>>   #define MMA8452_TRANSIENT_CFG			0x1d
>>> +#define  MMA8452_TRANSIENT_CFG_CHAN(chan)	BIT(chan + 1)
>>>   #define  MMA8452_TRANSIENT_CFG_HPF_BYP		BIT(0)
>>>   #define  MMA8452_TRANSIENT_CFG_ELE		BIT(4)
>>>   #define MMA8452_TRANSIENT_SRC			0x1e
>>> @@ -69,6 +71,7 @@
>>>   #define MMA8452_TRANSIENT_THS			0x1f
>>>   #define  MMA8452_TRANSIENT_THS_MASK		GENMASK(6, 0)
>>>   #define MMA8452_TRANSIENT_COUNT			0x20
>>> +#define MMA8452_TRANSIENT_CHAN_SHIFT 1
>>>   #define MMA8452_CTRL_REG1			0x2a
>>>   #define  MMA8452_CTRL_ACTIVE			BIT(0)
>>>   #define  MMA8452_CTRL_DR_MASK			GENMASK(5, 3)
>>> @@ -107,6 +110,26 @@ struct mma8452_data {
>>>   	const struct mma_chip_info *chip_info;
>>>   };
>>>   
>>> + /**
>>> +  * struct mma8452_event_regs - chip specific data related to
>events
>>> +  * @ev_cfg:			event config register address
>>> +  * @ev_src:			event source register address
>>> +  * @ev_ths:			event threshold register address
>>> +  * @ev_ths_mask:		mask for the threshold value
>>> +  * @ev_count:			event count (period) register address
>>> +  *
>>> +  * Since not all chips supported by the driver support comparing
>high pass
>>> +  * filtered data for events (interrupts), different interrupt
>sources are
>>> +  * used for different chips and the relevant registers are
>included here.
>>> +  */
>>> +struct mma8452_event_regs {
>>> +		u8 ev_cfg;
>>> +		u8 ev_src;
>>> +		u8 ev_ths;
>>> +		u8 ev_ths_mask;
>>> +		u8 ev_count;
>>> +};
>>> +
>>>   /**
>>>    * struct mma_chip_info - chip specific data
>>>    * @chip_id:			WHO_AM_I register's value
>>> @@ -116,40 +139,12 @@ struct mma8452_data {
>>>    * @mma_scales:			scale factors for converting register values
>>>    *				to m/s^2; 3 modes: 2g, 4g, 8g; 2 integers
>>>    *				per mode: m/s^2 and micro m/s^2
>>> - * @ev_cfg:			event config register address
>>> - * @ev_cfg_ele:			latch bit in event config register
>>> - * @ev_cfg_chan_shift:		number of the bit to enable events in X
>>> - *				direction; in event config register
>>> - * @ev_src:			event source register address
>>> - * @ev_src_xe:			bit in event source register that indicates
>>> - *				an event in X direction
>>> - * @ev_src_ye:			bit in event source register that indicates
>>> - *				an event in Y direction
>>> - * @ev_src_ze:			bit in event source register that indicates
>>> - *				an event in Z direction
>>> - * @ev_ths:			event threshold register address
>>> - * @ev_ths_mask:		mask for the threshold value
>>> - * @ev_count:			event count (period) register address
>>> - *
>>> - * Since not all chips supported by the driver support comparing
>high pass
>>> - * filtered data for events (interrupts), different interrupt
>sources are
>>> - * used for different chips and the relevant registers are included
>here.
>>>    */
>>>   struct mma_chip_info {
>>>   	u8 chip_id;
>>>   	const struct iio_chan_spec *channels;
>>>   	int num_channels;
>>>   	const int mma_scales[3][2];
>>> -	u8 ev_cfg;
>>> -	u8 ev_cfg_ele;
>>> -	u8 ev_cfg_chan_shift;
>>> -	u8 ev_src;
>>> -	u8 ev_src_xe;
>>> -	u8 ev_src_ye;
>>> -	u8 ev_src_ze;
>>> -	u8 ev_ths;
>>> -	u8 ev_ths_mask;
>>> -	u8 ev_count;
>>>   };
>>>   
>>>   enum {
>>> @@ -602,9 +597,8 @@ static int mma8452_set_power_mode(struct
>mma8452_data *data, u8 mode)
>>>   static int mma8452_freefall_mode_enabled(struct mma8452_data
>*data)
>>>   {
>>>   	int val;
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   
>>> -	val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>>> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
>>>   	if (val < 0)
>>>   		return val;
>>>   
>>> @@ -614,29 +608,28 @@ static int
>mma8452_freefall_mode_enabled(struct mma8452_data *data)
>>>   static int mma8452_set_freefall_mode(struct mma8452_data *data,
>bool state)
>>>   {
>>>   	int val;
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   
>>>   	if ((state && mma8452_freefall_mode_enabled(data)) ||
>>>   	    (!state && !(mma8452_freefall_mode_enabled(data))))
>>>   		return 0;
>>>   
>>> -	val = i2c_smbus_read_byte_data(data->client, chip->ev_cfg);
>>> +	val = i2c_smbus_read_byte_data(data->client, MMA8452_FF_MT_CFG);
>>>   	if (val < 0)
>>>   		return val;
>>>   
>>>   	if (state) {
>>> -		val |= BIT(idx_x + chip->ev_cfg_chan_shift);
>>> -		val |= BIT(idx_y + chip->ev_cfg_chan_shift);
>>> -		val |= BIT(idx_z + chip->ev_cfg_chan_shift);
>>> +		val |= BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val |= BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val |= BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
>>>   		val &= ~MMA8452_FF_MT_CFG_OAE;
>>>   	} else {
>>> -		val &= ~BIT(idx_x + chip->ev_cfg_chan_shift);
>>> -		val &= ~BIT(idx_y + chip->ev_cfg_chan_shift);
>>> -		val &= ~BIT(idx_z + chip->ev_cfg_chan_shift);
>>> +		val &= ~BIT(idx_x + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val &= ~BIT(idx_y + MMA8452_FF_MT_CHAN_SHIFT);
>>> +		val &= ~BIT(idx_z + MMA8452_FF_MT_CHAN_SHIFT);
>>>   		val |= MMA8452_FF_MT_CFG_OAE;
>>>   	}
>>>   
>>> -	return mma8452_change_config(data, chip->ev_cfg, val);
>>> +	return mma8452_change_config(data, MMA8452_FF_MT_CFG, val);
>>>   }
>>>   
>>>   static int mma8452_set_hp_filter_frequency(struct mma8452_data
>*data,
>>> @@ -740,6 +733,39 @@ static int mma8452_write_raw(struct iio_dev
>*indio_dev,
>>>   	return ret;
>>>   }
>>>   
>>> +static int mma8452_get_event_regs(const struct iio_chan_spec *chan,
>>> +	       enum iio_event_direction dir,
>>> +		   struct mma8452_event_regs *ev_regs
>>> +	       )
>>> +{
>>> +	if (!chan)
>>> +		return -EINVAL;
>>> +	switch (chan->type) {
>>> +	case IIO_ACCEL:
>>> +		switch (dir) {
>>> +		case IIO_EV_DIR_FALLING:
>>> +			ev_regs->ev_cfg = MMA8452_FF_MT_CFG;
>>> +			ev_regs->ev_src = MMA8452_FF_MT_SRC;
>>> +			ev_regs->ev_ths = MMA8452_FF_MT_THS;
>>> +			ev_regs->ev_ths_mask = MMA8452_FF_MT_THS_MASK;
>>> +			ev_regs->ev_count = MMA8452_FF_MT_COUNT;
>>> +			break;
>>> +		case IIO_EV_DIR_RISING:
>>> +			ev_regs->ev_cfg = MMA8452_TRANSIENT_CFG;
>>> +			ev_regs->ev_src = MMA8452_TRANSIENT_SRC;
>>> +			ev_regs->ev_ths = MMA8452_TRANSIENT_THS;
>>
>> ev_ths_mask is not set here
>>
>>> +			ev_regs->ev_count = MMA8452_TRANSIENT_COUNT;
>>> +			break;
>>> +		default:
>>> +			return -EINVAL;
>>> +		}
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +	return 0;
>> could replace 'breaks' with return 0 to save some lines and simplify
>> control flow
>>
>>> +}
>>> +
>>>   static int mma8452_read_thresh(struct iio_dev *indio_dev,
>>>   			       const struct iio_chan_spec *chan,
>>>   			       enum iio_event_type type,
>>> @@ -749,21 +775,23 @@ static int mma8452_read_thresh(struct iio_dev
>*indio_dev,
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>>   	int ret, us, power_mode;
>>> +	struct mma8452_event_regs ev_regs;
>>>   
>>> +	ret = mma8452_get_event_regs(chan, dir, &ev_regs);
>>> +	if (ret)
>>> +		return ret;
>>>   	switch (info) {
>>>   	case IIO_EV_INFO_VALUE:
>>> -		ret = i2c_smbus_read_byte_data(data->client,
>>> -					       data->chip_info->ev_ths);
>>> +		ret = i2c_smbus_read_byte_data(data->client, ev_regs.ev_ths);
>>>   		if (ret < 0)
>>>   			return ret;
>>>   
>>> -		*val = ret & data->chip_info->ev_ths_mask;
>>> +		*val = ret & ev_regs.ev_ths_mask;
>>>   
>>>   		return IIO_VAL_INT;
>>>   
>>>   	case IIO_EV_INFO_PERIOD:
>>> -		ret = i2c_smbus_read_byte_data(data->client,
>>> -					       data->chip_info->ev_count);
>>> +		ret = i2c_smbus_read_byte_data(data->client, ev_regs.ev_count);
>>>   		if (ret < 0)
>>>   			return ret;
>>>   
>>> @@ -809,14 +837,18 @@ static int mma8452_write_thresh(struct iio_dev
>*indio_dev,
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>>   	int ret, reg, steps;
>>> +	struct mma8452_event_regs ev_regs;
>>> +
>>> +	ret = mma8452_get_event_regs(chan, dir, &ev_regs);
>>> +	if (ret)
>>> +		return ret;
>>>   
>>>   	switch (info) {
>>>   	case IIO_EV_INFO_VALUE:
>>> -		if (val < 0 || val > MMA8452_TRANSIENT_THS_MASK)
>>> +		if (val < 0 || val > ev_regs.ev_ths_mask)
>>>   			return -EINVAL;
>>>   
>>> -		return mma8452_change_config(data, data->chip_info->ev_ths,
>>> -					     val);
>>> +		return mma8452_change_config(data, ev_regs.ev_ths, val);
>>>   
>>>   	case IIO_EV_INFO_PERIOD:
>>>   		ret = mma8452_get_power_mode(data);
>>> @@ -830,8 +862,7 @@ static int mma8452_write_thresh(struct iio_dev
>*indio_dev,
>>>   		if (steps < 0 || steps > 0xff)
>>>   			return -EINVAL;
>>>   
>>> -		return mma8452_change_config(data, data->chip_info->ev_count,
>>> -					     steps);
>>> +		return mma8452_change_config(data, ev_regs.ev_count, steps);
>>>   
>>>   	case IIO_EV_INFO_HIGH_PASS_FILTER_3DB:
>>>   		reg = i2c_smbus_read_byte_data(data->client,
>>> @@ -861,23 +892,23 @@ static int mma8452_read_event_config(struct
>iio_dev *indio_dev,
>>>   				     enum iio_event_direction dir)
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   	int ret;
>>>   
>>> -	switch (dir) {
>>> -	case IIO_EV_DIR_FALLING:
>>> -		return mma8452_freefall_mode_enabled(data);
>>> -	case IIO_EV_DIR_RISING:
>>> -		if (mma8452_freefall_mode_enabled(data))
>>> -			return 0;
>>> +	switch (chan->type) {
>>> +	case IIO_ACCEL:
>> this adds a check for chan-type == IIO_ACCESS, so strictly this would
>be
>> an unrelated change...
>>
>>> +		switch (dir) {
>>> +		case IIO_EV_DIR_FALLING:
>>> +			return mma8452_freefall_mode_enabled(data);
>>> +		case IIO_EV_DIR_RISING:
>>> +			ret = i2c_smbus_read_byte_data(data->client,
>MMA8452_TRANSIENT_CFG);
>>> +			if (ret < 0)
>>> +				return ret;
>>>   
>>> -		ret = i2c_smbus_read_byte_data(data->client,
>>> -					       data->chip_info->ev_cfg);
>>> -		if (ret < 0)
>>> -			return ret;
>>> +			return !!(ret & MMA8452_TRANSIENT_CFG_CHAN(chan->scan_index));
>>>   
>>> -		return !!(ret & BIT(chan->scan_index +
>>> -				    chip->ev_cfg_chan_shift));
>>> +		default:
>>> +			return -EINVAL;
>>> +		}
>>>   	default:
>>>   		return -EINVAL;
>>>   	}
>>> @@ -890,39 +921,33 @@ static int mma8452_write_event_config(struct
>iio_dev *indio_dev,
>>>   				      int state)
>>>   {
>>>   	struct mma8452_data *data = iio_priv(indio_dev);
>>> -	const struct mma_chip_info *chip = data->chip_info;
>>>   	int val, ret;
>>>   
>>>   	ret = mma8452_set_runtime_pm_state(data->client, state);
>>>   	if (ret)
>>>   		retu

-- 
Martin Kepplinger
http://martinkepplinger.com
sent from mobile
--
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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux