From: Jonathan Cameron <jic23@kernel.org>
To: Alexandru Ardelean <alexandru.ardelean@analog.com>
Cc: <linux-iio@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 08/10] iio: imu: adis16400: rework locks using ADIS library's state lock
Date: Sun, 6 Oct 2019 10:20:56 +0100 [thread overview]
Message-ID: <20191006102056.686fdd06@archlinux> (raw)
In-Reply-To: <20190926111812.15957-9-alexandru.ardelean@analog.com>
On Thu, 26 Sep 2019 14:18:10 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> This change removes the use of indio_dev's mlock in favor using the state
> lock from the ADIS library.
>
> The set_freq() & get_freq() hooks are unlocked, so they require specific
> locking. That is because in some cases the get_freq() hook is used in
> combination with adis16400_set_filter().
>
> In cases where only one read/write is done, the functions that hold the
> state lock are used.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.
Thanks,
Jonathan
> ---
> drivers/iio/imu/adis16400.c | 51 ++++++++++++++++++-------------------
> 1 file changed, 25 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
> index 0575ff706bd4..e042a2aabf6b 100644
> --- a/drivers/iio/imu/adis16400.c
> +++ b/drivers/iio/imu/adis16400.c
> @@ -162,6 +162,7 @@ struct adis16400_chip_info {
> unsigned int accel_scale_micro;
> int temp_scale_nano;
> int temp_offset;
> + /* set_freq() & get_freq() need to avoid using ADIS lib's state lock */
> int (*set_freq)(struct adis16400_state *st, unsigned int freq);
> int (*get_freq)(struct adis16400_state *st);
> };
> @@ -326,7 +327,7 @@ static int adis16334_get_freq(struct adis16400_state *st)
> int ret;
> uint16_t t;
>
> - ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> + ret = __adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> if (ret < 0)
> return ret;
>
> @@ -350,7 +351,7 @@ static int adis16334_set_freq(struct adis16400_state *st, unsigned int freq)
> t <<= ADIS16334_RATE_DIV_SHIFT;
> t |= ADIS16334_RATE_INT_CLK;
>
> - return adis_write_reg_16(&st->adis, ADIS16400_SMPL_PRD, t);
> + return __adis_write_reg_16(&st->adis, ADIS16400_SMPL_PRD, t);
> }
>
> static int adis16400_get_freq(struct adis16400_state *st)
> @@ -358,7 +359,7 @@ static int adis16400_get_freq(struct adis16400_state *st)
> int sps, ret;
> uint16_t t;
>
> - ret = adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> + ret = __adis_read_reg_16(&st->adis, ADIS16400_SMPL_PRD, &t);
> if (ret < 0)
> return ret;
>
> @@ -390,7 +391,7 @@ static int adis16400_set_freq(struct adis16400_state *st, unsigned int freq)
> else
> st->adis.spi->max_speed_hz = ADIS16400_SPI_FAST;
>
> - return adis_write_reg_8(&st->adis, ADIS16400_SMPL_PRD, val);
> + return __adis_write_reg_8(&st->adis, ADIS16400_SMPL_PRD, val);
> }
>
> static const unsigned int adis16400_3db_divisors[] = {
> @@ -404,7 +405,7 @@ static const unsigned int adis16400_3db_divisors[] = {
> [7] = 200, /* Not a valid setting */
> };
>
> -static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
> +static int __adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
> {
> struct adis16400_state *st = iio_priv(indio_dev);
> uint16_t val16;
> @@ -415,11 +416,11 @@ static int adis16400_set_filter(struct iio_dev *indio_dev, int sps, int val)
> break;
> }
>
> - ret = adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
> + ret = __adis_read_reg_16(&st->adis, ADIS16400_SENS_AVG, &val16);
> if (ret < 0)
> return ret;
>
> - ret = adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
> + ret = __adis_write_reg_16(&st->adis, ADIS16400_SENS_AVG,
> (val16 & ~0x07) | i);
> return ret;
> }
> @@ -507,32 +508,31 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan, int val, int val2, long info)
> {
> struct adis16400_state *st = iio_priv(indio_dev);
> + struct mutex *slock = &st->adis.state_lock;
> int ret, sps;
>
> switch (info) {
> case IIO_CHAN_INFO_CALIBBIAS:
> - mutex_lock(&indio_dev->mlock);
> ret = adis_write_reg_16(&st->adis,
> adis16400_addresses[chan->scan_index], val);
> - mutex_unlock(&indio_dev->mlock);
> return ret;
> case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> /*
> * Need to cache values so we can update if the frequency
> * changes.
> */
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(slock);
> st->filt_int = val;
> /* Work out update to current value */
> sps = st->variant->get_freq(st);
> if (sps < 0) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(slock);
> return sps;
> }
>
> - ret = adis16400_set_filter(indio_dev, sps,
> + ret = __adis16400_set_filter(indio_dev, sps,
> val * 1000 + val2 / 1000);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(slock);
> return ret;
> case IIO_CHAN_INFO_SAMP_FREQ:
> sps = val * 1000 + val2 / 1000;
> @@ -540,9 +540,9 @@ static int adis16400_write_raw(struct iio_dev *indio_dev,
> if (sps <= 0)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(slock);
> ret = st->variant->set_freq(st, sps);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(slock);
> return ret;
> default:
> return -EINVAL;
> @@ -553,6 +553,7 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan, int *val, int *val2, long info)
> {
> struct adis16400_state *st = iio_priv(indio_dev);
> + struct mutex *slock = &st->adis.state_lock;
> int16_t val16;
> int ret;
>
> @@ -596,10 +597,8 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
> case IIO_CHAN_INFO_CALIBBIAS:
> - mutex_lock(&indio_dev->mlock);
> ret = adis_read_reg_16(&st->adis,
> adis16400_addresses[chan->scan_index], &val16);
> - mutex_unlock(&indio_dev->mlock);
> if (ret)
> return ret;
> val16 = sign_extend32(val16, 11);
> @@ -610,27 +609,27 @@ static int adis16400_read_raw(struct iio_dev *indio_dev,
> *val = st->variant->temp_offset;
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(slock);
> /* Need both the number of taps and the sampling frequency */
> - ret = adis_read_reg_16(&st->adis,
> + ret = __adis_read_reg_16(&st->adis,
> ADIS16400_SENS_AVG,
> &val16);
> if (ret < 0) {
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(slock);
> return ret;
> }
> ret = st->variant->get_freq(st);
> - if (ret >= 0) {
> - ret /= adis16400_3db_divisors[val16 & 0x07];
> - *val = ret / 1000;
> - *val2 = (ret % 1000) * 1000;
> - }
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(slock);
> if (ret < 0)
> return ret;
> + ret /= adis16400_3db_divisors[val16 & 0x07];
> + *val = ret / 1000;
> + *val2 = (ret % 1000) * 1000;
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_CHAN_INFO_SAMP_FREQ:
> + mutex_lock(slock);
> ret = st->variant->get_freq(st);
> + mutex_unlock(slock);
> if (ret < 0)
> return ret;
> *val = ret / 1000;
next prev parent reply other threads:[~2019-10-06 9:21 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-26 11:18 [PATCH 00/10] iio: imu: adis: cleanup lock usage Alexandru Ardelean
2019-09-26 11:18 ` [PATCH 01/10] iio: imu: adis: rename txrx_lock -> state_lock Alexandru Ardelean
2019-10-06 8:53 ` Jonathan Cameron
2019-10-06 9:06 ` Jonathan Cameron
2019-10-07 10:18 ` Ardelean, Alexandru
2019-09-26 11:18 ` [PATCH 02/10] iio: imu: adis: add unlocked read/write function versions Alexandru Ardelean
2019-10-06 9:12 ` Jonathan Cameron
2019-10-07 21:16 ` Jonathan Cameron
2019-10-08 6:54 ` Ardelean, Alexandru
2019-10-08 8:47 ` Ardelean, Alexandru
2019-10-08 8:58 ` Ardelean, Alexandru
2019-10-12 13:40 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 03/10] iio: imu: adis[16480]: group RW into a single lock in adis_enable_irq() Alexandru Ardelean
2019-10-06 9:13 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 04/10] iio: imu: adis: create an unlocked version of adis_check_status() Alexandru Ardelean
2019-10-06 9:13 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 05/10] iio: imu: adis: create an unlocked version of adis_reset() Alexandru Ardelean
2019-10-06 9:19 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 06/10] iio: imu: adis: protect initial startup routine with state lock Alexandru Ardelean
2019-10-06 9:20 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 07/10] iio: imu: adis: group single conversion under a single " Alexandru Ardelean
2019-10-06 9:20 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 08/10] iio: imu: adis16400: rework locks using ADIS library's " Alexandru Ardelean
2019-10-06 9:20 ` Jonathan Cameron [this message]
2019-09-26 11:18 ` [PATCH 09/10] iio: gyro: adis16136: " Alexandru Ardelean
2019-10-06 9:22 ` Jonathan Cameron
2019-09-26 11:18 ` [PATCH 10/10] iio: imu: adis16480: use state lock for filter freq set Alexandru Ardelean
2019-10-06 9:24 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191006102056.686fdd06@archlinux \
--to=jic23@kernel.org \
--cc=alexandru.ardelean@analog.com \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).