On Sat, 2018-06-30 at 18:30 +0100, Jonathan Cameron wrote:
> 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.
Ack.
Will come back with a V2.
Thanks
Alex
>
> Jonathan
>
> > ---
> >
> > v1 -> v2:
> > * added Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxx
> > m>
> >
> > 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_C
> > LK_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);
>
> ��.n��������+%������w��{.n�����{��(��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
