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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ffc140c7-3072-870a-3cec-587e8553d1e2@posteo.de>
Date:   Sun, 10 Sep 2017 08:36:43 +0200
From:   Martin Kepplinger <martink@...teo.de>
To:     Harinath Nampally <harinath922@...il.com>, jic23@...nel.org
Cc:     knaack.h@....de, lars@...afoo.de, pmeerw@...erw.net,
        gregkh@...uxfoundation.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, amsfield22@...il.com
Subject: Re: [PATCH v6] iio: accel: mma8452: improvements to handle multiple
 events

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@...il.com>
> ---

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@...teo.de>


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;
> +	}
> +}
> +