On Sat, 2018-07-21 at 17:40 +0100, Jonathan Cameron wrote:
> On Wed, 18 Jul 2018 10:29:53 +0300
> Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx> wrote:
>
> > From: Lars-Peter Clausen <lars@xxxxxxxxxx>
> >
> > This is also part of a long term effort to make the use of mlock opaque
> > and
> > single purpose.
> >
> > This lock is required for accessing device registers. Some operations
> > require multiple consecutive R/W operations, during which the device
> > shouldn't be interrupted.
> >
> > Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>
> Hi Alexandru. Lars pointed out I was simply wrong in my complaints with
> the previous description - it was correct in that case - we need perhaps
> to expand the description to include what you have here as well.
>
> Given I doubt you care too much on the wording I've fused the two.
>
> /*
> * Lock for accessing device registers. Some operations require
> * multiple consecutive R/W operations, during which the device
> * shouldn't be interrupted. The buffers are also shared across
> * all operations so need to be protected on stand alone reads and
> * writes.
> */
>
> Applied with that minor addition. Thanks for persistence on this!
Ack.
I wasn't sure what the final preference was for this, so I sent a V4 in
case that was more preferred.
Thanks
Alex
>
> Jonathan
>
> > ---
> >
> > V3 -> V4:
> > * updated comment/description of the lock
> >
> > drivers/iio/frequency/ad9523.c | 32 ++++++++++++++++++++++----------
> > 1 file changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/frequency/ad9523.c
> > b/drivers/iio/frequency/ad9523.c
> > index ddb6a334ae68..2ab175a0664d 100644
> > --- a/drivers/iio/frequency/ad9523.c
> > +++ b/drivers/iio/frequency/ad9523.c
> > @@ -274,6 +274,13 @@ struct ad9523_state {
> > unsigned long vco_out_freq[AD9523_NUM_CLK_SRC];
> > unsigned char vco_out_map[AD9523_NUM_CHAN_ALT_C
> > LK_SRC];
> >
> > + /*
> > + * Lock for accessing device registers. Some operations
> > require
> > + * multiple consecutive R/W operations, during which the
> > device
> > + * shouldn't be interrupted.
> > + */
> > + struct mutex lock;
> > +
> > /*
> > * DMA (thus cache coherency maintenance) requires the
> > * transfer buffers to live in their own cache lines.
> > @@ -500,6 +507,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 +518,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 +529,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 +540,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 +632,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 +668,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 +714,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 +722,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 +738,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 +977,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�����٥
