* Re: [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race
[not found] <20210119112211.26404-1-chi962464zy@163.com>
@ 2021-01-19 14:54 ` Tom Rix
2021-01-20 1:48 ` Guoqing Chi
0 siblings, 1 reply; 5+ messages in thread
From: Tom Rix @ 2021-01-19 14:54 UTC (permalink / raw)
To: Guoqing Chi
Cc: martin.blumenstingl, linux-kernel, chiguoqing, huyue, zhangwen,
linux-iio
On 1/19/21 3:22 AM, Guoqing Chi wrote:
> From: chiguoqing <chi962464zy@163.com>
>
> Adding mutex_lock, when read and write reg need to use this lock to
> avoid race.
>
> Signed-off-by: Guoqing Chi <chiguoqing@yulong.com>
> ---
> v2:Follow write function to fix read function.
> Adding mutex init in core probe function.
> Adding break in switch case at read and write function.
>
> drivers/iio/imu/bmi160/bmi160.h | 2 ++
> drivers/iio/imu/bmi160/bmi160_core.c | 34 +++++++++++++++++++---------
> 2 files changed, 25 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index 32c2ea2d7112..0c189a8b5b53 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -3,9 +3,11 @@
> #define BMI160_H_
>
> #include <linux/iio/iio.h>
> +#include <linux/mutex.h>
> #include <linux/regulator/consumer.h>
>
> struct bmi160_data {
> + struct mutex lock;
> struct regmap *regmap;
> struct iio_trigger *trig;
> struct regulator_bulk_data supplies[2];
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 290b5ef83f77..e303378f4841 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -452,26 +452,32 @@ static int bmi160_read_raw(struct iio_dev *indio_dev,
> int ret;
> struct bmi160_data *data = iio_priv(indio_dev);
>
> + mutex_lock(&data->lock);
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> ret = bmi160_get_data(data, chan->type, chan->channel2, val);
> - if (ret)
> - return ret;
> - return IIO_VAL_INT;
> + if (!ret)
> + ret = IIO_VAL_INT;
> + break;
> case IIO_CHAN_INFO_SCALE:
> *val = 0;
> ret = bmi160_get_scale(data,
> bmi160_to_sensor(chan->type), val2);
> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> + if (!ret)
> + ret = IIO_VAL_INT_PLUS_MICRO;
Looking better, another question..
Why does the write() function return the results directly while the read() function
translates them to other values ?
Tom
> + break;
> case IIO_CHAN_INFO_SAMP_FREQ:
> ret = bmi160_get_odr(data, bmi160_to_sensor(chan->type),
> val, val2);
> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> + if (!ret)
> + ret = IIO_VAL_INT_PLUS_MICRO;
> + break;
> default:
> - return -EINVAL;
> + ret = -EINVAL;
> }
> + mutex_unlock(&data->lock);
>
> - return 0;
> + return ret;
> }
>
> static int bmi160_write_raw(struct iio_dev *indio_dev,
> @@ -479,19 +485,24 @@ static int bmi160_write_raw(struct iio_dev *indio_dev,
> int val, int val2, long mask)
> {
> struct bmi160_data *data = iio_priv(indio_dev);
> + int result;
>
> + mutex_lock(&data->lock);
> switch (mask) {
> case IIO_CHAN_INFO_SCALE:
> - return bmi160_set_scale(data,
> + result = bmi160_set_scale(data,
> bmi160_to_sensor(chan->type), val2);
> + break;
> case IIO_CHAN_INFO_SAMP_FREQ:
> - return bmi160_set_odr(data, bmi160_to_sensor(chan->type),
> + result = bmi160_set_odr(data, bmi160_to_sensor(chan->type),
> val, val2);
> + break;
> default:
> - return -EINVAL;
> + result = -EINVAL;
> }
> + mutex_unlock(&data->lock);
>
> - return 0;
> + return result;
> }
>
> static
> @@ -838,6 +849,7 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> return -ENOMEM;
>
> data = iio_priv(indio_dev);
> + mutex_init(&data->lock);
> dev_set_drvdata(dev, indio_dev);
> data->regmap = regmap;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race
2021-01-19 14:54 ` [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race Tom Rix
@ 2021-01-20 1:48 ` Guoqing Chi
2021-01-20 15:06 ` Tom Rix
0 siblings, 1 reply; 5+ messages in thread
From: Guoqing Chi @ 2021-01-20 1:48 UTC (permalink / raw)
To: Tom Rix
Cc: martin.blumenstingl, linux-kernel, chiguoqing, huyue2, zhangwen,
linux-iio
On Tue, 19 Jan 2021 06:54:45 -0800
Tom Rix <trix@redhat.com> wrote:
> On 1/19/21 3:22 AM, Guoqing Chi wrote:
> > From: chiguoqing <chi962464zy@163.com>
> >
> > Adding mutex_lock, when read and write reg need to use this lock to
> > avoid race.
> >
> > Signed-off-by: Guoqing Chi <chiguoqing@yulong.com>
> > ---
> > v2:Follow write function to fix read function.
> > Adding mutex init in core probe function.
> > Adding break in switch case at read and write function.
> >
> > drivers/iio/imu/bmi160/bmi160.h | 2 ++
> > drivers/iio/imu/bmi160/bmi160_core.c | 34
> > +++++++++++++++++++--------- 2 files changed, 25 insertions(+), 11
> > deletions(-)
> >
> > diff --git a/drivers/iio/imu/bmi160/bmi160.h
> > b/drivers/iio/imu/bmi160/bmi160.h index 32c2ea2d7112..0c189a8b5b53
> > 100644 --- a/drivers/iio/imu/bmi160/bmi160.h
> > +++ b/drivers/iio/imu/bmi160/bmi160.h
> > @@ -3,9 +3,11 @@
> > #define BMI160_H_
> >
> > #include <linux/iio/iio.h>
> > +#include <linux/mutex.h>
> > #include <linux/regulator/consumer.h>
> >
> > struct bmi160_data {
> > + struct mutex lock;
> > struct regmap *regmap;
> > struct iio_trigger *trig;
> > struct regulator_bulk_data supplies[2];
> > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c
> > b/drivers/iio/imu/bmi160/bmi160_core.c index
> > 290b5ef83f77..e303378f4841 100644 ---
> > a/drivers/iio/imu/bmi160/bmi160_core.c +++
> > b/drivers/iio/imu/bmi160/bmi160_core.c @@ -452,26 +452,32 @@ static
> > int bmi160_read_raw(struct iio_dev *indio_dev, int ret;
> > struct bmi160_data *data = iio_priv(indio_dev);
> >
> > + mutex_lock(&data->lock);
> > switch (mask) {
> > case IIO_CHAN_INFO_RAW:
> > ret = bmi160_get_data(data, chan->type,
> > chan->channel2, val);
> > - if (ret)
> > - return ret;
> > - return IIO_VAL_INT;
> > + if (!ret)
> > + ret = IIO_VAL_INT;
> > + break;
> > case IIO_CHAN_INFO_SCALE:
> > *val = 0;
> > ret = bmi160_get_scale(data,
> > bmi160_to_sensor(chan->type),
> > val2);
> > - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> > + if (!ret)
> > + ret = IIO_VAL_INT_PLUS_MICRO;
>
> Looking better, another question..
>
> Why does the write() function return the results directly while the
> read() function
>
> translates them to other values ?
>
> Tom
It is original design in this driver. In order to
differentiate raw to scale and SAMP_FREQ, while the scale and SAMP_FREQ
are needless. I think log information can be added for this purpose,
and return results directly.
It is not change the return values for my modify.It's best to keep the
original design.Is that all right?
Guoqing Chi
>
> > + break;
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > ret = bmi160_get_odr(data,
> > bmi160_to_sensor(chan->type), val, val2);
> > - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> > + if (!ret)
> > + ret = IIO_VAL_INT_PLUS_MICRO;
> > + break;
> > default:
> > - return -EINVAL;
> > + ret = -EINVAL;
> > }
> > + mutex_unlock(&data->lock);
> >
> > - return 0;
> > + return ret;
> > }
> >
> > static int bmi160_write_raw(struct iio_dev *indio_dev,
> > @@ -479,19 +485,24 @@ static int bmi160_write_raw(struct iio_dev
> > *indio_dev, int val, int val2, long mask)
> > {
> > struct bmi160_data *data = iio_priv(indio_dev);
> > + int result;
> >
> > + mutex_lock(&data->lock);
> > switch (mask) {
> > case IIO_CHAN_INFO_SCALE:
> > - return bmi160_set_scale(data,
> > + result = bmi160_set_scale(data,
> > bmi160_to_sensor(chan->type),
> > val2);
> > + break;
> > case IIO_CHAN_INFO_SAMP_FREQ:
> > - return bmi160_set_odr(data,
> > bmi160_to_sensor(chan->type),
> > + result = bmi160_set_odr(data,
> > bmi160_to_sensor(chan->type), val, val2);
> > + break;
> > default:
> > - return -EINVAL;
> > + result = -EINVAL;
> > }
> > + mutex_unlock(&data->lock);
> >
> > - return 0;
> > + return result;
> > }
> >
> > static
> > @@ -838,6 +849,7 @@ int bmi160_core_probe(struct device *dev,
> > struct regmap *regmap, return -ENOMEM;
> >
> > data = iio_priv(indio_dev);
> > + mutex_init(&data->lock);
> > dev_set_drvdata(dev, indio_dev);
> > data->regmap = regmap;
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race
2021-01-20 1:48 ` Guoqing Chi
@ 2021-01-20 15:06 ` Tom Rix
2021-01-23 15:26 ` Jonathan Cameron
0 siblings, 1 reply; 5+ messages in thread
From: Tom Rix @ 2021-01-20 15:06 UTC (permalink / raw)
To: Guoqing Chi
Cc: martin.blumenstingl, linux-kernel, chiguoqing, huyue2, zhangwen,
linux-iio
On 1/19/21 5:48 PM, Guoqing Chi wrote:
> On Tue, 19 Jan 2021 06:54:45 -0800
> Tom Rix <trix@redhat.com> wrote:
>
>> On 1/19/21 3:22 AM, Guoqing Chi wrote:
>>> From: chiguoqing <chi962464zy@163.com>
>>>
>>> Adding mutex_lock, when read and write reg need to use this lock to
>>> avoid race.
>>>
>>> Signed-off-by: Guoqing Chi <chiguoqing@yulong.com>
>>> ---
>>> v2:Follow write function to fix read function.
>>> Adding mutex init in core probe function.
>>> Adding break in switch case at read and write function.
>>>
>>> drivers/iio/imu/bmi160/bmi160.h | 2 ++
>>> drivers/iio/imu/bmi160/bmi160_core.c | 34
>>> +++++++++++++++++++--------- 2 files changed, 25 insertions(+), 11
>>> deletions(-)
>>>
>>> diff --git a/drivers/iio/imu/bmi160/bmi160.h
>>> b/drivers/iio/imu/bmi160/bmi160.h index 32c2ea2d7112..0c189a8b5b53
>>> 100644 --- a/drivers/iio/imu/bmi160/bmi160.h
>>> +++ b/drivers/iio/imu/bmi160/bmi160.h
>>> @@ -3,9 +3,11 @@
>>> #define BMI160_H_
>>>
>>> #include <linux/iio/iio.h>
>>> +#include <linux/mutex.h>
>>> #include <linux/regulator/consumer.h>
>>>
>>> struct bmi160_data {
>>> + struct mutex lock;
>>> struct regmap *regmap;
>>> struct iio_trigger *trig;
>>> struct regulator_bulk_data supplies[2];
>>> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c
>>> b/drivers/iio/imu/bmi160/bmi160_core.c index
>>> 290b5ef83f77..e303378f4841 100644 ---
>>> a/drivers/iio/imu/bmi160/bmi160_core.c +++
>>> b/drivers/iio/imu/bmi160/bmi160_core.c @@ -452,26 +452,32 @@ static
>>> int bmi160_read_raw(struct iio_dev *indio_dev, int ret;
>>> struct bmi160_data *data = iio_priv(indio_dev);
>>>
>>> + mutex_lock(&data->lock);
>>> switch (mask) {
>>> case IIO_CHAN_INFO_RAW:
>>> ret = bmi160_get_data(data, chan->type,
>>> chan->channel2, val);
>>> - if (ret)
>>> - return ret;
>>> - return IIO_VAL_INT;
>>> + if (!ret)
>>> + ret = IIO_VAL_INT;
>>> + break;
>>> case IIO_CHAN_INFO_SCALE:
>>> *val = 0;
>>> ret = bmi160_get_scale(data,
>>> bmi160_to_sensor(chan->type),
>>> val2);
>>> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
>>> + if (!ret)
>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>> Looking better, another question..
>>
>> Why does the write() function return the results directly while the
>> read() function
>>
>> translates them to other values ?
>>
>> Tom
> It is original design in this driver. In order to
> differentiate raw to scale and SAMP_FREQ, while the scale and SAMP_FREQ
> are needless. I think log information can be added for this purpose,
> and return results directly.
> It is not change the return values for my modify.It's best to keep the
> original design.Is that all right?
Ok.
Reviewed-by: Tom Rix <trix@redhat.com>
> Guoqing Chi
>>> + break;
>>> case IIO_CHAN_INFO_SAMP_FREQ:
>>> ret = bmi160_get_odr(data,
>>> bmi160_to_sensor(chan->type), val, val2);
>>> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
>>> + if (!ret)
>>> + ret = IIO_VAL_INT_PLUS_MICRO;
>>> + break;
>>> default:
>>> - return -EINVAL;
>>> + ret = -EINVAL;
>>> }
>>> + mutex_unlock(&data->lock);
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static int bmi160_write_raw(struct iio_dev *indio_dev,
>>> @@ -479,19 +485,24 @@ static int bmi160_write_raw(struct iio_dev
>>> *indio_dev, int val, int val2, long mask)
>>> {
>>> struct bmi160_data *data = iio_priv(indio_dev);
>>> + int result;
>>>
>>> + mutex_lock(&data->lock);
>>> switch (mask) {
>>> case IIO_CHAN_INFO_SCALE:
>>> - return bmi160_set_scale(data,
>>> + result = bmi160_set_scale(data,
>>> bmi160_to_sensor(chan->type),
>>> val2);
>>> + break;
>>> case IIO_CHAN_INFO_SAMP_FREQ:
>>> - return bmi160_set_odr(data,
>>> bmi160_to_sensor(chan->type),
>>> + result = bmi160_set_odr(data,
>>> bmi160_to_sensor(chan->type), val, val2);
>>> + break;
>>> default:
>>> - return -EINVAL;
>>> + result = -EINVAL;
>>> }
>>> + mutex_unlock(&data->lock);
>>>
>>> - return 0;
>>> + return result;
>>> }
>>>
>>> static
>>> @@ -838,6 +849,7 @@ int bmi160_core_probe(struct device *dev,
>>> struct regmap *regmap, return -ENOMEM;
>>>
>>> data = iio_priv(indio_dev);
>>> + mutex_init(&data->lock);
>>> dev_set_drvdata(dev, indio_dev);
>>> data->regmap = regmap;
>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race
2021-01-20 15:06 ` Tom Rix
@ 2021-01-23 15:26 ` Jonathan Cameron
2021-01-26 2:58 ` Guoqing Chi
0 siblings, 1 reply; 5+ messages in thread
From: Jonathan Cameron @ 2021-01-23 15:26 UTC (permalink / raw)
To: Tom Rix
Cc: Guoqing Chi, martin.blumenstingl, linux-kernel, chiguoqing,
huyue2, zhangwen, linux-iio
On Wed, 20 Jan 2021 07:06:23 -0800
Tom Rix <trix@redhat.com> wrote:
> On 1/19/21 5:48 PM, Guoqing Chi wrote:
> > On Tue, 19 Jan 2021 06:54:45 -0800
> > Tom Rix <trix@redhat.com> wrote:
> >
> >> On 1/19/21 3:22 AM, Guoqing Chi wrote:
> >>> From: chiguoqing <chi962464zy@163.com>
> >>>
> >>> Adding mutex_lock, when read and write reg need to use this lock to
> >>> avoid race.
> >>>
> >>> Signed-off-by: Guoqing Chi <chiguoqing@yulong.com>
> >>> ---
> >>> v2:Follow write function to fix read function.
> >>> Adding mutex init in core probe function.
> >>> Adding break in switch case at read and write function.
> >>>
> >>> drivers/iio/imu/bmi160/bmi160.h | 2 ++
> >>> drivers/iio/imu/bmi160/bmi160_core.c | 34
> >>> +++++++++++++++++++--------- 2 files changed, 25 insertions(+), 11
> >>> deletions(-)
> >>>
> >>> diff --git a/drivers/iio/imu/bmi160/bmi160.h
> >>> b/drivers/iio/imu/bmi160/bmi160.h index 32c2ea2d7112..0c189a8b5b53
> >>> 100644 --- a/drivers/iio/imu/bmi160/bmi160.h
> >>> +++ b/drivers/iio/imu/bmi160/bmi160.h
> >>> @@ -3,9 +3,11 @@
> >>> #define BMI160_H_
> >>>
> >>> #include <linux/iio/iio.h>
> >>> +#include <linux/mutex.h>
> >>> #include <linux/regulator/consumer.h>
> >>>
> >>> struct bmi160_data {
> >>> + struct mutex lock;
> >>> struct regmap *regmap;
> >>> struct iio_trigger *trig;
> >>> struct regulator_bulk_data supplies[2];
> >>> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c
> >>> b/drivers/iio/imu/bmi160/bmi160_core.c index
> >>> 290b5ef83f77..e303378f4841 100644 ---
> >>> a/drivers/iio/imu/bmi160/bmi160_core.c +++
> >>> b/drivers/iio/imu/bmi160/bmi160_core.c @@ -452,26 +452,32 @@ static
> >>> int bmi160_read_raw(struct iio_dev *indio_dev, int ret;
> >>> struct bmi160_data *data = iio_priv(indio_dev);
> >>>
> >>> + mutex_lock(&data->lock);
> >>> switch (mask) {
> >>> case IIO_CHAN_INFO_RAW:
> >>> ret = bmi160_get_data(data, chan->type,
> >>> chan->channel2, val);
> >>> - if (ret)
> >>> - return ret;
> >>> - return IIO_VAL_INT;
> >>> + if (!ret)
> >>> + ret = IIO_VAL_INT;
> >>> + break;
> >>> case IIO_CHAN_INFO_SCALE:
> >>> *val = 0;
> >>> ret = bmi160_get_scale(data,
> >>> bmi160_to_sensor(chan->type),
> >>> val2);
> >>> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> >>> + if (!ret)
> >>> + ret = IIO_VAL_INT_PLUS_MICRO;
> >> Looking better, another question..
> >>
> >> Why does the write() function return the results directly while the
> >> read() function
> >>
> >> translates them to other values ?
> >>
> >> Tom
> > It is original design in this driver. In order to
> > differentiate raw to scale and SAMP_FREQ, while the scale and SAMP_FREQ
> > are needless. I think log information can be added for this purpose,
> > and return results directly.
> > It is not change the return values for my modify.It's best to keep the
> > original design.Is that all right?
>
> Ok.
>
> Reviewed-by: Tom Rix <trix@redhat.com>
Hi Guoqing Chi,
For some reason the original patch email (start of this thread) never
made it to my inbox or indeed the archive at lore.kernel.org.
Please resend (picking up Tom's reviewed by) and make sure to cc
linux-iio@vger.kernel.org + jic23@kernel.org
Then check if they make it to lore.kernel.org as that should highlight
any issues where it is getting blocked etc.
Thanks,
Jonathan
>
> > Guoqing Chi
> >>> + break;
> >>> case IIO_CHAN_INFO_SAMP_FREQ:
> >>> ret = bmi160_get_odr(data,
> >>> bmi160_to_sensor(chan->type), val, val2);
> >>> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> >>> + if (!ret)
> >>> + ret = IIO_VAL_INT_PLUS_MICRO;
> >>> + break;
> >>> default:
> >>> - return -EINVAL;
> >>> + ret = -EINVAL;
> >>> }
> >>> + mutex_unlock(&data->lock);
> >>>
> >>> - return 0;
> >>> + return ret;
> >>> }
> >>>
> >>> static int bmi160_write_raw(struct iio_dev *indio_dev,
> >>> @@ -479,19 +485,24 @@ static int bmi160_write_raw(struct iio_dev
> >>> *indio_dev, int val, int val2, long mask)
> >>> {
> >>> struct bmi160_data *data = iio_priv(indio_dev);
> >>> + int result;
> >>>
> >>> + mutex_lock(&data->lock);
> >>> switch (mask) {
> >>> case IIO_CHAN_INFO_SCALE:
> >>> - return bmi160_set_scale(data,
> >>> + result = bmi160_set_scale(data,
> >>> bmi160_to_sensor(chan->type),
> >>> val2);
> >>> + break;
> >>> case IIO_CHAN_INFO_SAMP_FREQ:
> >>> - return bmi160_set_odr(data,
> >>> bmi160_to_sensor(chan->type),
> >>> + result = bmi160_set_odr(data,
> >>> bmi160_to_sensor(chan->type), val, val2);
> >>> + break;
> >>> default:
> >>> - return -EINVAL;
> >>> + result = -EINVAL;
> >>> }
> >>> + mutex_unlock(&data->lock);
> >>>
> >>> - return 0;
> >>> + return result;
> >>> }
> >>>
> >>> static
> >>> @@ -838,6 +849,7 @@ int bmi160_core_probe(struct device *dev,
> >>> struct regmap *regmap, return -ENOMEM;
> >>>
> >>> data = iio_priv(indio_dev);
> >>> + mutex_init(&data->lock);
> >>> dev_set_drvdata(dev, indio_dev);
> >>> data->regmap = regmap;
> >>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race
2021-01-23 15:26 ` Jonathan Cameron
@ 2021-01-26 2:58 ` Guoqing Chi
0 siblings, 0 replies; 5+ messages in thread
From: Guoqing Chi @ 2021-01-26 2:58 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Tom Rix, martin.blumenstingl, linux-kernel, chiguoqing, huyue2,
zhangwen, linux-iio
On Sat, 23 Jan 2021 15:26:59 +0000
Jonathan Cameron <jic23@kernel.org> wrote:
> On Wed, 20 Jan 2021 07:06:23 -0800
> Tom Rix <trix@redhat.com> wrote:
>
> > On 1/19/21 5:48 PM, Guoqing Chi wrote:
> > > On Tue, 19 Jan 2021 06:54:45 -0800
> > > Tom Rix <trix@redhat.com> wrote:
> > >
> > >> On 1/19/21 3:22 AM, Guoqing Chi wrote:
> > >>> From: chiguoqing <chi962464zy@163.com>
> > >>>
> > >>> Adding mutex_lock, when read and write reg need to use this
> > >>> lock to avoid race.
> > >>>
> > >>> Signed-off-by: Guoqing Chi <chiguoqing@yulong.com>
> > >>> ---
> > >>> v2:Follow write function to fix read function.
> > >>> Adding mutex init in core probe function.
> > >>> Adding break in switch case at read and write function.
> > >>>
> > >>> drivers/iio/imu/bmi160/bmi160.h | 2 ++
> > >>> drivers/iio/imu/bmi160/bmi160_core.c | 34
> > >>> +++++++++++++++++++--------- 2 files changed, 25 insertions(+),
> > >>> 11 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iio/imu/bmi160/bmi160.h
> > >>> b/drivers/iio/imu/bmi160/bmi160.h index
> > >>> 32c2ea2d7112..0c189a8b5b53 100644 ---
> > >>> a/drivers/iio/imu/bmi160/bmi160.h +++
> > >>> b/drivers/iio/imu/bmi160/bmi160.h @@ -3,9 +3,11 @@
> > >>> #define BMI160_H_
> > >>>
> > >>> #include <linux/iio/iio.h>
> > >>> +#include <linux/mutex.h>
> > >>> #include <linux/regulator/consumer.h>
> > >>>
> > >>> struct bmi160_data {
> > >>> + struct mutex lock;
> > >>> struct regmap *regmap;
> > >>> struct iio_trigger *trig;
> > >>> struct regulator_bulk_data supplies[2];
> > >>> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c
> > >>> b/drivers/iio/imu/bmi160/bmi160_core.c index
> > >>> 290b5ef83f77..e303378f4841 100644 ---
> > >>> a/drivers/iio/imu/bmi160/bmi160_core.c +++
> > >>> b/drivers/iio/imu/bmi160/bmi160_core.c @@ -452,26 +452,32 @@
> > >>> static int bmi160_read_raw(struct iio_dev *indio_dev, int ret;
> > >>> struct bmi160_data *data = iio_priv(indio_dev);
> > >>>
> > >>> + mutex_lock(&data->lock);
> > >>> switch (mask) {
> > >>> case IIO_CHAN_INFO_RAW:
> > >>> ret = bmi160_get_data(data, chan->type,
> > >>> chan->channel2, val);
> > >>> - if (ret)
> > >>> - return ret;
> > >>> - return IIO_VAL_INT;
> > >>> + if (!ret)
> > >>> + ret = IIO_VAL_INT;
> > >>> + break;
> > >>> case IIO_CHAN_INFO_SCALE:
> > >>> *val = 0;
> > >>> ret = bmi160_get_scale(data,
> > >>> bmi160_to_sensor(chan->type),
> > >>> val2);
> > >>> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> > >>> + if (!ret)
> > >>> + ret = IIO_VAL_INT_PLUS_MICRO;
> > >> Looking better, another question..
> > >>
> > >> Why does the write() function return the results directly while
> > >> the read() function
> > >>
> > >> translates them to other values ?
> > >>
> > >> Tom
> > > It is original design in this driver. In order to
> > > differentiate raw to scale and SAMP_FREQ, while the scale and
> > > SAMP_FREQ are needless. I think log information can be added for
> > > this purpose, and return results directly.
> > > It is not change the return values for my modify.It's best to
> > > keep the original design.Is that all right?
> >
> > Ok.
> >
> > Reviewed-by: Tom Rix <trix@redhat.com>
>
> Hi Guoqing Chi,
>
> For some reason the original patch email (start of this thread) never
> made it to my inbox or indeed the archive at lore.kernel.org.
>
> Please resend (picking up Tom's reviewed by) and make sure to cc
> linux-iio@vger.kernel.org + jic23@kernel.org
>
> Then check if they make it to lore.kernel.org as that should highlight
> any issues where it is getting blocked etc.
>
> Thanks,
>
> Jonathan
Hi Jonathan,
I have resend patch V2(picking up Tom's reviewed by) for add
linux-iio@vger.kernel.org + jic23@kernel.org.
Thanks.
>
> >
> > > Guoqing Chi
> > >>> + break;
> > >>> case IIO_CHAN_INFO_SAMP_FREQ:
> > >>> ret = bmi160_get_odr(data,
> > >>> bmi160_to_sensor(chan->type), val, val2);
> > >>> - return ret ? ret : IIO_VAL_INT_PLUS_MICRO;
> > >>> + if (!ret)
> > >>> + ret = IIO_VAL_INT_PLUS_MICRO;
> > >>> + break;
> > >>> default:
> > >>> - return -EINVAL;
> > >>> + ret = -EINVAL;
> > >>> }
> > >>> + mutex_unlock(&data->lock);
> > >>>
> > >>> - return 0;
> > >>> + return ret;
> > >>> }
> > >>>
> > >>> static int bmi160_write_raw(struct iio_dev *indio_dev,
> > >>> @@ -479,19 +485,24 @@ static int bmi160_write_raw(struct iio_dev
> > >>> *indio_dev, int val, int val2, long mask)
> > >>> {
> > >>> struct bmi160_data *data = iio_priv(indio_dev);
> > >>> + int result;
> > >>>
> > >>> + mutex_lock(&data->lock);
> > >>> switch (mask) {
> > >>> case IIO_CHAN_INFO_SCALE:
> > >>> - return bmi160_set_scale(data,
> > >>> + result = bmi160_set_scale(data,
> > >>> bmi160_to_sensor(chan->type),
> > >>> val2);
> > >>> + break;
> > >>> case IIO_CHAN_INFO_SAMP_FREQ:
> > >>> - return bmi160_set_odr(data,
> > >>> bmi160_to_sensor(chan->type),
> > >>> + result = bmi160_set_odr(data,
> > >>> bmi160_to_sensor(chan->type), val, val2);
> > >>> + break;
> > >>> default:
> > >>> - return -EINVAL;
> > >>> + result = -EINVAL;
> > >>> }
> > >>> + mutex_unlock(&data->lock);
> > >>>
> > >>> - return 0;
> > >>> + return result;
> > >>> }
> > >>>
> > >>> static
> > >>> @@ -838,6 +849,7 @@ int bmi160_core_probe(struct device *dev,
> > >>> struct regmap *regmap, return -ENOMEM;
> > >>>
> > >>> data = iio_priv(indio_dev);
> > >>> + mutex_init(&data->lock);
> > >>> dev_set_drvdata(dev, indio_dev);
> > >>> data->regmap = regmap;
> > >>>
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-01-26 5:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210119112211.26404-1-chi962464zy@163.com>
2021-01-19 14:54 ` [PATCH v2] iio: imu: bmi160: add mutex_lock for avoiding race Tom Rix
2021-01-20 1:48 ` Guoqing Chi
2021-01-20 15:06 ` Tom Rix
2021-01-23 15:26 ` Jonathan Cameron
2021-01-26 2:58 ` Guoqing Chi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).