On Wed, Feb 20, 2019 at 8:24 PM <jic23@xxxxxxxxxx> wrote:
>
Hey,
One comment inline.
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
>
> indio_dev->mlock is intended to protect state transitions in
> the core. It's scope is tightly defined. For device specific
> uses such as those made here, we should define a local lock
> allowing the scope of the lock to be defined near to what it
> is protecting.
>
> These mlock changes can be non obvious, but given we don't do
> anything other than direct for DACs, these ones are easy to do.
>
> If anyone wants to help with this particular effort it would
> be most welcome!
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
> drivers/iio/dac/ad5064.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iio/dac/ad5064.c b/drivers/iio/dac/ad5064.c
> index 2f98cb2a3b96..6c3ba143839b 100644
> --- a/drivers/iio/dac/ad5064.c
> +++ b/drivers/iio/dac/ad5064.c
> @@ -112,6 +112,8 @@ struct ad5064_state {
> bool use_internal_vref;
>
> ad5064_write_func write;
> + /* Lock used to maintain consistency between cached and dev state */
> + struct mutex lock;
>
> /*
> * DMA (thus cache coherency maintenance) requires the
> @@ -248,11 +250,11 @@ static int ad5064_set_powerdown_mode(struct iio_dev *indio_dev,
> struct ad5064_state *st = iio_priv(indio_dev);
> int ret;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> st->pwr_down_mode[chan->channel] = mode + 1;
>
> ret = ad5064_sync_powerdown_mode(st, chan);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret;
> }
> @@ -291,11 +293,11 @@ static ssize_t ad5064_write_dac_powerdown(struct iio_dev *indio_dev,
> if (ret)
> return ret;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> st->pwr_down[chan->channel] = pwr_down;
>
> ret = ad5064_sync_powerdown_mode(st, chan);
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret ? ret : len;
> }
>
> @@ -349,12 +351,12 @@ static int ad5064_write_raw(struct iio_dev *indio_dev,
> if (val >= (1 << chan->scan_type.realbits) || val < 0)
> return -EINVAL;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = ad5064_write(st, AD5064_CMD_WRITE_INPUT_N_UPDATE_N,
> chan->address, val, chan->scan_type.shift);
> if (ret == 0)
> st->dac_cache[chan->channel] = val;
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> break;
> default:
> ret = -EINVAL;
> @@ -856,6 +858,7 @@ static int ad5064_probe(struct device *dev, enum ad5064_type type,
> return -ENOMEM;
>
> st = iio_priv(indio_dev);
> + mutex_init(&st->lock);
An equivalent `mutex_destroy()` call would be a good idea in ad5064_remove()
> dev_set_drvdata(dev, indio_dev);
>
> st->chip_info = &ad5064_chip_info_tbl[type];
> --
> 2.20.1
>
