On Mon, 25 Jun 2018 11:01:17 +0300
Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
> From: Lars-Peter Clausen <lars@xxxxxxxxxx>
>
> This is part of a long term effort to make the use of mlock opaque and
> single purpose.
>
> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
Patch is fine, but I'd like to see precise documentation for the
scope of the lock. These things can be awfully 'fuzzy' so good
to define this explicitly.
Jonathan
> ---
>
> v1 -> v2:
> * added Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>
> drivers/iio/frequency/ad9523.c | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/frequency/ad9523.c b/drivers/iio/frequency/ad9523.c
> index ddb6a334ae68..1b70e8a13338 100644
> --- a/drivers/iio/frequency/ad9523.c
> +++ b/drivers/iio/frequency/ad9523.c
> @@ -274,6 +274,8 @@ struct ad9523_state {
> unsigned long vco_out_freq[AD9523_NUM_CLK_SRC];
> unsigned char vco_out_map[AD9523_NUM_CHAN_ALT_CLK_SRC];
>
I'd like to see a precise definition of what this lock is intended
to protect. Not to mention checkpatch will moan about it I think
if there is nothing here ;)
> + struct mutex lock;
> +
> /*
> * DMA (thus cache coherency maintenance) requires the
> * transfer buffers to live in their own cache lines.
> @@ -500,6 +502,7 @@ static ssize_t ad9523_store(struct device *dev,
> {
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> + struct ad9523_state *st = iio_priv(indio_dev);
> bool state;
> int ret;
>
> @@ -510,7 +513,7 @@ static ssize_t ad9523_store(struct device *dev,
> if (!state)
> return 0;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> switch ((u32)this_attr->address) {
> case AD9523_SYNC:
> ret = ad9523_sync(indio_dev);
> @@ -521,7 +524,7 @@ static ssize_t ad9523_store(struct device *dev,
> default:
> ret = -ENODEV;
> }
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret ? ret : len;
> }
> @@ -532,15 +535,16 @@ static ssize_t ad9523_show(struct device *dev,
> {
> struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> + struct ad9523_state *st = iio_priv(indio_dev);
> int ret;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = ad9523_read(indio_dev, AD9523_READBACK_0);
> if (ret >= 0) {
> ret = sprintf(buf, "%d\n", !!(ret & (1 <<
> (u32)this_attr->address)));
> }
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret;
> }
> @@ -623,9 +627,9 @@ static int ad9523_read_raw(struct iio_dev *indio_dev,
> unsigned int code;
> int ret;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = ad9523_read(indio_dev, AD9523_CHANNEL_CLOCK_DIST(chan->channel));
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> if (ret < 0)
> return ret;
> @@ -659,7 +663,7 @@ static int ad9523_write_raw(struct iio_dev *indio_dev,
> unsigned int reg;
> int ret, tmp, code;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> ret = ad9523_read(indio_dev, AD9523_CHANNEL_CLOCK_DIST(chan->channel));
> if (ret < 0)
> goto out;
> @@ -705,7 +709,7 @@ static int ad9523_write_raw(struct iio_dev *indio_dev,
>
> ad9523_io_update(indio_dev);
> out:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
> return ret;
> }
>
> @@ -713,9 +717,10 @@ static int ad9523_reg_access(struct iio_dev *indio_dev,
> unsigned int reg, unsigned int writeval,
> unsigned int *readval)
> {
> + struct ad9523_state *st = iio_priv(indio_dev);
> int ret;
>
> - mutex_lock(&indio_dev->mlock);
> + mutex_lock(&st->lock);
> if (readval == NULL) {
> ret = ad9523_write(indio_dev, reg | AD9523_R1B, writeval);
> ad9523_io_update(indio_dev);
> @@ -728,7 +733,7 @@ static int ad9523_reg_access(struct iio_dev *indio_dev,
> }
>
> out_unlock:
> - mutex_unlock(&indio_dev->mlock);
> + mutex_unlock(&st->lock);
>
> return ret;
> }
> @@ -967,6 +972,8 @@ static int ad9523_probe(struct spi_device *spi)
>
> st = iio_priv(indio_dev);
>
> + mutex_init(&st->lock);
> +
> st->reg = devm_regulator_get(&spi->dev, "vcc");
> if (!IS_ERR(st->reg)) {
> ret = regulator_enable(st->reg);
--
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
