Re: [RFC/PATCH v3] staging: iio: update locking method during frequency writes

[Gargi Sharma <gs051095@gmail.com>]

On Thu, Mar 30, 2017 at 11:58 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 30/03/17 19:25, Alison Schofield wrote:
>> On Thu, Mar 30, 2017 at 07:22:49PM +0100, Jonathan Cameron wrote:
>>> On 30/03/17 10:33, Gargi Sharma wrote:
>>>> The driver needs to insure atomicity during frequency
>>>> changes of bus and device. The iiodev->mlock as used
>>>> was not doing that. Replace it with the drivers existing
>>>> buffer lock and introduce an auxiliary spi_write() that does
>>>> not hold the lock.
>>>>
>>>> Signed-off-by: Gargi Sharma <gs051095@gmail.com>
>>> Only one comment on this.  Why an RFC?  I nearly
>>> ignored this in my sweep of easy patches to pick up
>>> this evening purely because I thought there might still
>>> be something that needed real thought in it.
>>
>> Oh, that was my bad influence!
>> We were reviewing proposals for these in Outreachy and I'd
>> suggested the RFC label for the proposals.  Thanks for
>> addressing it.
>> alisons
> Was fair enough for round one - though saying why it might
> need comments in the patch description is always good.
>
> By V3 with all positive comments, probably fine to loose the
> RFC :)

I was skeptical of the RFC tag hence put the PATCH in subject prefix
as well. :) Anyways, lesson learnt for future!

Thanks,
Gargi
>>
>>>
>>> It's a clean nice patch that I don't think any substantial
>>> questions have been raised against - so just submit it as
>>> patch. As an RFC it is underselling itself!
>>>
>>> Anyhow, never mind, I did look and have applied it ;)
>>>
>>> Applied to the togreg branch of iio.git and pushed out
>>> as testing for the autobuilders to play with it.
>>>
>>> Thanks.
>>>
>>> Jonathan
>>>> ---
>>>> Changes in v3:
>>>>         - Removed unnecessary ret variable inside
>>>>           __ade7754_spi_write_reg_8 function.
>>>> Changes in v2:
>>>>         - Added auxiliary function.
>>>>         - Updated the buf_lock comment appropriately.
>>>> ---
>>>>  drivers/staging/iio/meter/ade7754.c | 24 ++++++++++++++++--------
>>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
>>>> index c8d2d4c..91f8740 100644
>>>> --- a/drivers/staging/iio/meter/ade7754.c
>>>> +++ b/drivers/staging/iio/meter/ade7754.c
>>>> @@ -97,7 +97,7 @@
>>>>  /**
>>>>   * struct ade7754_state - device instance specific data
>>>>   * @us:                    actual spi_device
>>>> - * @buf_lock:              mutex to protect tx and rx
>>>> + * @buf_lock:              mutex to protect tx, rx and write frequency
>>>>   * @tx:                    transmit buffer
>>>>   * @rx:                    receive buffer
>>>>   **/
>>>> @@ -108,17 +108,25 @@ struct ade7754_state {
>>>>     u8                      rx[ADE7754_MAX_RX];
>>>>  };
>>>>
>>>> -static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>> +/* Unlocked version of ade7754_spi_write_reg_8 function */
>>>> +static int __ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>>  {
>>>> -   int ret;
>>>>     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>>     struct ade7754_state *st = iio_priv(indio_dev);
>>>>
>>>> -   mutex_lock(&st->buf_lock);
>>>>     st->tx[0] = ADE7754_WRITE_REG(reg_address);
>>>>     st->tx[1] = val;
>>>> +   return spi_write(st->us, st->tx, 2);
>>>> +}
>>>>
>>>> -   ret = spi_write(st->us, st->tx, 2);
>>>> +static int ade7754_spi_write_reg_8(struct device *dev, u8 reg_address, u8 val)
>>>> +{
>>>> +   int ret;
>>>> +   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>>> +   struct ade7754_state *st = iio_priv(indio_dev);
>>>> +
>>>> +   mutex_lock(&st->buf_lock);
>>>> +   ret = __ade7754_spi_write_reg_8(dev, reg_address, val);
>>>>     mutex_unlock(&st->buf_lock);
>>>>
>>>>     return ret;
>>>> @@ -513,7 +521,7 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>>>>     if (!val)
>>>>             return -EINVAL;
>>>>
>>>> -   mutex_lock(&indio_dev->mlock);
>>>> +   mutex_lock(&st->buf_lock);
>>>>
>>>>     t = 26000 / val;
>>>>     if (t > 0)
>>>> @@ -531,10 +539,10 @@ static ssize_t ade7754_write_frequency(struct device *dev,
>>>>     reg &= ~(3 << 3);
>>>>     reg |= t << 3;
>>>>
>>>> -   ret = ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>>>> +   ret = __ade7754_spi_write_reg_8(dev, ADE7754_WAVMODE, reg);
>>>>
>>>>  out:
>>>> -   mutex_unlock(&indio_dev->mlock);
>>>> +   mutex_unlock(&st->buf_lock);
>>>>
>>>>     return ret ? ret : len;
>>>>  }
>>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>