Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/11/2 下午5:24, Jonathan Cameron wrote:
On Fri, 2 Nov 2018 15:55:27 +0800
Song Qiang wrote:
On 2018/10/21 下午10:14, Jonathan Cameron wrote:
On Thu, 18 Oct 2018 16:24:15 +0800
Song Qiang wrote:
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
What about BIT(0) | BIT(2)?
Now you can do it like you have here and on that one corner case let the iio
core
demux code take care of it, but then you will need to provide
available_scan_masks
so the core knows it needs to handle this case.
This confused me a little. The available_scan_masks I was using is {BIT(0) |
BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2), etc.
Since Phil mentioned he would like this to reduce bus usage as much as we can
and I want it, too, I think these three circumstances can be read consecutively
while others can be read one axis at a time. So I plan to let BIT(0) | BIT(2)
fall into the 'default' section, which reads axis one by one.
My question is, since this handles every possible combination, do I still need
to list every available scan masks in available_scan_masks?
Ah. I see, I'd missed that the default was picking up that case as well as the
single axes. It would be interesting to sanity check if it is quicker on
a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
and drop the middle value (which would be done using available scan_masks)
or to just do two independent reads.
(I would guess it is worth reading the 'dead' axis).
All other problems will be fixed in the next patch.
yours,
Song Qiang
...
Thanks,
Jonathan
I tested this two ways of getting data with the following code snippet:
u8 buffer[9];
struct timeval timebefore, timeafter;
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read with dead axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 3);
if (ret < 0)
goto unlock_return;
ret = regmap_bulk_read(regmap, RM3100_REG_MZ2, buffer + 6, 3);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read two single axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
And get this result:
[ 161.264777] read with dead axis time: 883
[ 161.270621] read two single axis time: 1359
[ 161.575134] read with dead axis time: 852
[ 161.580973] read two single axis time: 1356
[ 161.895704] read with dead axis time: 854
[ 161.903744] read two single axis time: 3540
[ 162.223600] read with dead axis time: 853
[ 162.229451] read two single axis time: 1363
[ 162.591227] read with dead axis time: 850
[ 162.597630] read two single axis time: 1555
[ 162.920102] read with dead axis time: 852
[ 162.926467] read two single axis time: 1534
[ 163.303121] read with dead axis time: 881
[ 163.308997] read two single axis time: 1390
[ 163.711004] read with dead axis time: 861
It seems like you're right! Reading consecutively 9 bytes does save a lot time
compared to read 3 bytes twice.
I've done this stuff before ;) We had this on the adis16365 parts back
in the early days of IIO. A worse case as it has a lot more channels,
but otherwise similar from what I recall.
It would be an interesting exercise to trace those paths and find out the
balance between real hardware stuff we can't change and potential software
overheads.
Chances are this is mostly 'real' stuff though but would be great to
confirm this. It's
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/11/2 下午5:24, Jonathan Cameron wrote:
On Fri, 2 Nov 2018 15:55:27 +0800
Song Qiang wrote:
On 2018/10/21 下午10:14, Jonathan Cameron wrote:
On Thu, 18 Oct 2018 16:24:15 +0800
Song Qiang wrote:
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
What about BIT(0) | BIT(2)?
Now you can do it like you have here and on that one corner case let the iio
core
demux code take care of it, but then you will need to provide
available_scan_masks
so the core knows it needs to handle this case.
This confused me a little. The available_scan_masks I was using is {BIT(0) |
BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2), etc.
Since Phil mentioned he would like this to reduce bus usage as much as we can
and I want it, too, I think these three circumstances can be read consecutively
while others can be read one axis at a time. So I plan to let BIT(0) | BIT(2)
fall into the 'default' section, which reads axis one by one.
My question is, since this handles every possible combination, do I still need
to list every available scan masks in available_scan_masks?
Ah. I see, I'd missed that the default was picking up that case as well as the
single axes. It would be interesting to sanity check if it is quicker on
a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
and drop the middle value (which would be done using available scan_masks)
or to just do two independent reads.
(I would guess it is worth reading the 'dead' axis).
All other problems will be fixed in the next patch.
yours,
Song Qiang
...
Thanks,
Jonathan
I tested this two ways of getting data with the following code snippet:
u8 buffer[9];
struct timeval timebefore, timeafter;
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read with dead axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 3);
if (ret < 0)
goto unlock_return;
ret = regmap_bulk_read(regmap, RM3100_REG_MZ2, buffer + 6, 3);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read two single axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
And get this result:
[ 161.264777] read with dead axis time: 883
[ 161.270621] read two single axis time: 1359
[ 161.575134] read with dead axis time: 852
[ 161.580973] read two single axis time: 1356
[ 161.895704] read with dead axis time: 854
[ 161.903744] read two single axis time: 3540
[ 162.223600] read with dead axis time: 853
[ 162.229451] read two single axis time: 1363
[ 162.591227] read with dead axis time: 850
[ 162.597630] read two single axis time: 1555
[ 162.920102] read with dead axis time: 852
[ 162.926467] read two single axis time: 1534
[ 163.303121] read with dead axis time: 881
[ 163.308997] read two single axis time: 1390
[ 163.711004] read with dead axis time: 861
It seems like you're right! Reading consecutively 9 bytes does save a lot time
compared to read 3 bytes twice.
I've done this stuff before ;) We had this on the adis16365 parts back
in the early days of IIO. A worse case as it has a lot more channels,
but otherwise similar from what I recall.
It would be an interesting exercise to trace those paths and find out the
balance between real hardware stuff we can't change and potential software
overheads.
Chances are this is mostly 'real' stuff though but would be great to
confirm this. It's
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Fri, 2 Nov 2018 15:55:27 +0800
Song Qiang wrote:
> On 2018/10/21 下午10:14, Jonathan Cameron wrote:
> > On Thu, 18 Oct 2018 16:24:15 +0800
> > Song Qiang wrote:
> >
> > ...
> +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> +{
> +struct iio_poll_func *pf = p;
> +struct iio_dev *indio_dev = pf->indio_dev;
> +unsigned long scan_mask = *indio_dev->active_scan_mask;
> +unsigned int mask_len = indio_dev->masklength;
> +struct rm3100_data *data = iio_priv(indio_dev);
> +struct regmap *regmap = data->regmap;
> +int ret, i, bit;
> +
> +mutex_lock(>lock);
> +switch (scan_mask) {
> +case BIT(0) | BIT(1) | BIT(2):
> +ret = regmap_bulk_read(regmap, RM3100_REG_MX2,
> data->buffer, 9);
> +mutex_unlock(>lock);
> +if (ret < 0)
> +goto done;
> +break;
> +case BIT(0) | BIT(1):
> +ret = regmap_bulk_read(regmap, RM3100_REG_MX2,
> data->buffer, 6);
> +mutex_unlock(>lock);
> +if (ret < 0)
> +goto done;
> +break;
> +case BIT(1) | BIT(2):
> +ret = regmap_bulk_read(regmap, RM3100_REG_MY2,
> data->buffer, 6);
> +mutex_unlock(>lock);
> +if (ret < 0)
> +goto done;
> +break;
> >>> What about BIT(0) | BIT(2)?
> >>>
> >>> Now you can do it like you have here and on that one corner case let the
> >>> iio core
> >>> demux code take care of it, but then you will need to provide
> >>> available_scan_masks
> >>> so the core knows it needs to handle this case.
> >>>
> >> This confused me a little. The available_scan_masks I was using is {BIT(0)
> >> |
> >> BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it
> >> to
> >> handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2),
> >> etc.
> >> Since Phil mentioned he would like this to reduce bus usage as much as we
> >> can
> >> and I want it, too, I think these three circumstances can be read
> >> consecutively
> >> while others can be read one axis at a time. So I plan to let BIT(0) |
> >> BIT(2)
> >> fall into the 'default' section, which reads axis one by one.
> >>
> >> My question is, since this handles every possible combination, do I still
> >> need
> >> to list every available scan masks in available_scan_masks?
> > Ah. I see, I'd missed that the default was picking up that case as well as
> > the
> > single axes. It would be interesting to sanity check if it is quicker on
> > a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
> > and drop the middle value (which would be done using available scan_masks)
> > or to just do two independent reads.
> >
> > (I would guess it is worth reading the 'dead' axis).
> >
> >>
> >> All other problems will be fixed in the next patch.
> >>
> >> yours,
> >>
> >> Song Qiang
> >>
> >>
> >> ...
> > Thanks,
> >
> > Jonathan
>
> I tested this two ways of getting data with the following code snippet:
>
>
> u8 buffer[9];
> struct timeval timebefore, timeafter;
>
> do_gettimeofday();
> ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
> if (ret < 0)
> goto unlock_return;
> do_gettimeofday();
> printk(KERN_INFO "read with dead axis time: %ld",
> timeafter.tv_sec * 100 + timeafter.tv_usec -
> timebefore.tv_sec * 100 - timebefore.tv_usec);
> do_gettimeofday();
>
> ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 3);
> if (ret < 0)
> goto unlock_return;
> ret = regmap_bulk_read(regmap, RM3100_REG_MZ2, buffer + 6, 3);
> if (ret < 0)
> goto unlock_return;
> do_gettimeofday();
> printk(KERN_INFO "read two single axis time: %ld",
> timeafter.tv_sec * 100 + timeafter.tv_usec -
> timebefore.tv_sec * 100 - timebefore.tv_usec);
>
>
> And get this result:
>
> [ 161.264777] read with dead axis time: 883
> [ 161.270621] read two single axis time: 1359
> [ 161.575134] read with dead axis time: 852
> [ 161.580973] read two single axis time: 1356
> [ 161.895704] read with dead axis time: 854
> [ 161.903744] read two single axis time: 3540
> [ 162.223600] read with dead axis time: 853
> [ 162.229451] read two single axis time: 1363
> [ 162.591227] read with dead axis time: 850
> [ 162.597630] read two single axis time: 1555
> [ 162.920102] read with dead axis time: 852
> [ 162.926467] read two single axis time: 1534
> [ 163.303121] read with dead axis time: 881
> [ 163.308997] read two single axis time: 1390
> [ 163.711004] read with dead axis time: 861
>
>
> It seems like
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Fri, 2 Nov 2018 15:55:27 +0800
Song Qiang wrote:
> On 2018/10/21 下午10:14, Jonathan Cameron wrote:
> > On Thu, 18 Oct 2018 16:24:15 +0800
> > Song Qiang wrote:
> >
> > ...
> +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> +{
> +struct iio_poll_func *pf = p;
> +struct iio_dev *indio_dev = pf->indio_dev;
> +unsigned long scan_mask = *indio_dev->active_scan_mask;
> +unsigned int mask_len = indio_dev->masklength;
> +struct rm3100_data *data = iio_priv(indio_dev);
> +struct regmap *regmap = data->regmap;
> +int ret, i, bit;
> +
> +mutex_lock(>lock);
> +switch (scan_mask) {
> +case BIT(0) | BIT(1) | BIT(2):
> +ret = regmap_bulk_read(regmap, RM3100_REG_MX2,
> data->buffer, 9);
> +mutex_unlock(>lock);
> +if (ret < 0)
> +goto done;
> +break;
> +case BIT(0) | BIT(1):
> +ret = regmap_bulk_read(regmap, RM3100_REG_MX2,
> data->buffer, 6);
> +mutex_unlock(>lock);
> +if (ret < 0)
> +goto done;
> +break;
> +case BIT(1) | BIT(2):
> +ret = regmap_bulk_read(regmap, RM3100_REG_MY2,
> data->buffer, 6);
> +mutex_unlock(>lock);
> +if (ret < 0)
> +goto done;
> +break;
> >>> What about BIT(0) | BIT(2)?
> >>>
> >>> Now you can do it like you have here and on that one corner case let the
> >>> iio core
> >>> demux code take care of it, but then you will need to provide
> >>> available_scan_masks
> >>> so the core knows it needs to handle this case.
> >>>
> >> This confused me a little. The available_scan_masks I was using is {BIT(0)
> >> |
> >> BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it
> >> to
> >> handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2),
> >> etc.
> >> Since Phil mentioned he would like this to reduce bus usage as much as we
> >> can
> >> and I want it, too, I think these three circumstances can be read
> >> consecutively
> >> while others can be read one axis at a time. So I plan to let BIT(0) |
> >> BIT(2)
> >> fall into the 'default' section, which reads axis one by one.
> >>
> >> My question is, since this handles every possible combination, do I still
> >> need
> >> to list every available scan masks in available_scan_masks?
> > Ah. I see, I'd missed that the default was picking up that case as well as
> > the
> > single axes. It would be interesting to sanity check if it is quicker on
> > a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
> > and drop the middle value (which would be done using available scan_masks)
> > or to just do two independent reads.
> >
> > (I would guess it is worth reading the 'dead' axis).
> >
> >>
> >> All other problems will be fixed in the next patch.
> >>
> >> yours,
> >>
> >> Song Qiang
> >>
> >>
> >> ...
> > Thanks,
> >
> > Jonathan
>
> I tested this two ways of getting data with the following code snippet:
>
>
> u8 buffer[9];
> struct timeval timebefore, timeafter;
>
> do_gettimeofday();
> ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
> if (ret < 0)
> goto unlock_return;
> do_gettimeofday();
> printk(KERN_INFO "read with dead axis time: %ld",
> timeafter.tv_sec * 100 + timeafter.tv_usec -
> timebefore.tv_sec * 100 - timebefore.tv_usec);
> do_gettimeofday();
>
> ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 3);
> if (ret < 0)
> goto unlock_return;
> ret = regmap_bulk_read(regmap, RM3100_REG_MZ2, buffer + 6, 3);
> if (ret < 0)
> goto unlock_return;
> do_gettimeofday();
> printk(KERN_INFO "read two single axis time: %ld",
> timeafter.tv_sec * 100 + timeafter.tv_usec -
> timebefore.tv_sec * 100 - timebefore.tv_usec);
>
>
> And get this result:
>
> [ 161.264777] read with dead axis time: 883
> [ 161.270621] read two single axis time: 1359
> [ 161.575134] read with dead axis time: 852
> [ 161.580973] read two single axis time: 1356
> [ 161.895704] read with dead axis time: 854
> [ 161.903744] read two single axis time: 3540
> [ 162.223600] read with dead axis time: 853
> [ 162.229451] read two single axis time: 1363
> [ 162.591227] read with dead axis time: 850
> [ 162.597630] read two single axis time: 1555
> [ 162.920102] read with dead axis time: 852
> [ 162.926467] read two single axis time: 1534
> [ 163.303121] read with dead axis time: 881
> [ 163.308997] read two single axis time: 1390
> [ 163.711004] read with dead axis time: 861
>
>
> It seems like
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/10/21 下午10:14, Jonathan Cameron wrote:
On Thu, 18 Oct 2018 16:24:15 +0800
Song Qiang wrote:
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
What about BIT(0) | BIT(2)?
Now you can do it like you have here and on that one corner case let the iio
core
demux code take care of it, but then you will need to provide
available_scan_masks
so the core knows it needs to handle this case.
This confused me a little. The available_scan_masks I was using is {BIT(0) |
BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2), etc.
Since Phil mentioned he would like this to reduce bus usage as much as we can
and I want it, too, I think these three circumstances can be read consecutively
while others can be read one axis at a time. So I plan to let BIT(0) | BIT(2)
fall into the 'default' section, which reads axis one by one.
My question is, since this handles every possible combination, do I still need
to list every available scan masks in available_scan_masks?
Ah. I see, I'd missed that the default was picking up that case as well as the
single axes. It would be interesting to sanity check if it is quicker on
a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
and drop the middle value (which would be done using available scan_masks)
or to just do two independent reads.
(I would guess it is worth reading the 'dead' axis).
All other problems will be fixed in the next patch.
yours,
Song Qiang
...
Thanks,
Jonathan
I tested this two ways of getting data with the following code snippet:
u8 buffer[9];
struct timeval timebefore, timeafter;
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read with dead axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 3);
if (ret < 0)
goto unlock_return;
ret = regmap_bulk_read(regmap, RM3100_REG_MZ2, buffer + 6, 3);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read two single axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
And get this result:
[ 161.264777] read with dead axis time: 883
[ 161.270621] read two single axis time: 1359
[ 161.575134] read with dead axis time: 852
[ 161.580973] read two single axis time: 1356
[ 161.895704] read with dead axis time: 854
[ 161.903744] read two single axis time: 3540
[ 162.223600] read with dead axis time: 853
[ 162.229451] read two single axis time: 1363
[ 162.591227] read with dead axis time: 850
[ 162.597630] read two single axis time: 1555
[ 162.920102] read with dead axis time: 852
[ 162.926467] read two single axis time: 1534
[ 163.303121] read with dead axis time: 881
[ 163.308997] read two single axis time: 1390
[ 163.711004] read with dead axis time: 861
It seems like you're right! Reading consecutively 9 bytes does save a lot time
compared to read 3 bytes twice.
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/10/21 下午10:14, Jonathan Cameron wrote:
On Thu, 18 Oct 2018 16:24:15 +0800
Song Qiang wrote:
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
What about BIT(0) | BIT(2)?
Now you can do it like you have here and on that one corner case let the iio
core
demux code take care of it, but then you will need to provide
available_scan_masks
so the core knows it needs to handle this case.
This confused me a little. The available_scan_masks I was using is {BIT(0) |
BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2), etc.
Since Phil mentioned he would like this to reduce bus usage as much as we can
and I want it, too, I think these three circumstances can be read consecutively
while others can be read one axis at a time. So I plan to let BIT(0) | BIT(2)
fall into the 'default' section, which reads axis one by one.
My question is, since this handles every possible combination, do I still need
to list every available scan masks in available_scan_masks?
Ah. I see, I'd missed that the default was picking up that case as well as the
single axes. It would be interesting to sanity check if it is quicker on
a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
and drop the middle value (which would be done using available scan_masks)
or to just do two independent reads.
(I would guess it is worth reading the 'dead' axis).
All other problems will be fixed in the next patch.
yours,
Song Qiang
...
Thanks,
Jonathan
I tested this two ways of getting data with the following code snippet:
u8 buffer[9];
struct timeval timebefore, timeafter;
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 9);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read with dead axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
do_gettimeofday();
ret = regmap_bulk_read(regmap, RM3100_REG_MX2, buffer, 3);
if (ret < 0)
goto unlock_return;
ret = regmap_bulk_read(regmap, RM3100_REG_MZ2, buffer + 6, 3);
if (ret < 0)
goto unlock_return;
do_gettimeofday();
printk(KERN_INFO "read two single axis time: %ld",
timeafter.tv_sec * 100 + timeafter.tv_usec -
timebefore.tv_sec * 100 - timebefore.tv_usec);
And get this result:
[ 161.264777] read with dead axis time: 883
[ 161.270621] read two single axis time: 1359
[ 161.575134] read with dead axis time: 852
[ 161.580973] read two single axis time: 1356
[ 161.895704] read with dead axis time: 854
[ 161.903744] read two single axis time: 3540
[ 162.223600] read with dead axis time: 853
[ 162.229451] read two single axis time: 1363
[ 162.591227] read with dead axis time: 850
[ 162.597630] read two single axis time: 1555
[ 162.920102] read with dead axis time: 852
[ 162.926467] read two single axis time: 1534
[ 163.303121] read with dead axis time: 881
[ 163.308997] read two single axis time: 1390
[ 163.711004] read with dead axis time: 861
It seems like you're right! Reading consecutively 9 bytes does save a lot time
compared to read 3 bytes twice.
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Thu, 18 Oct 2018 16:24:15 +0800
Song Qiang wrote:
> On 2018/10/13 下午6:19, Jonathan Cameron wrote:
> > On Fri, 12 Oct 2018 15:35:36 +0800
> > Song Qiang wrote:
> >
> >> PNI RM3100 is a high resolution, large signal immunity magnetometer,
> >> composed of 3 single sensors and a processing chip with a MagI2C
> >> interface.
> >>
> >> Following functions are available:
> >> - Single-shot measurement from
> >> /sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
> >> - Triggerd buffer measurement.
> >> - DRDY pin for data ready trigger.
> >> - Both i2c and spi interface are supported.
> >> - Both interrupt and polling measurement is supported, depends on if
> >> the 'interrupts' in DT is declared.
> >>
> >> Signed-off-by: Song Qiang
> > A few questions for you (getting very close to being good to go btw!)
> >
> > Why do we have the 3second additional wait for conversions? I know we
> > rarely wait that long, but still seems excessive.
> >
> > Few more comments inline.
> >
> > Thanks,
> >
> > Jonathan
>
> Hi Jonathan,
>
>
> The measurement time of this device varies from 1.7ms to 13 seconds, 3
> seconds
> is just a number in the middle between them. This may be worth discussing,
> hoping to get a better solution from the community.
We should 'know' which of those it will be though as I assume it is dependent
on the device configuration which we control.
So waiting for say, double, the expected time should be sufficient to detect
that things have gone horribly wrong.
>
>
> >> ---
> >> MAINTAINERS| 7 +
> >> drivers/iio/magnetometer/Kconfig | 29 ++
> >> drivers/iio/magnetometer/Makefile | 4 +
> >> drivers/iio/magnetometer/rm3100-core.c | 627 +
> >> drivers/iio/magnetometer/rm3100-i2c.c | 58 +++
> >> drivers/iio/magnetometer/rm3100-spi.c | 64 +++
> >> drivers/iio/magnetometer/rm3100.h | 17 +
> >> 7 files changed, 806 insertions(+)
> >> create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> >> create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> >> create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> >> create mode 100644 drivers/iio/magnetometer/rm3100.h
> >>
>
> ...
>
> >
> >> +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> >> +{
> >> + struct iio_poll_func *pf = p;
> >> + struct iio_dev *indio_dev = pf->indio_dev;
> >> + unsigned long scan_mask = *indio_dev->active_scan_mask;
> >> + unsigned int mask_len = indio_dev->masklength;
> >> + struct rm3100_data *data = iio_priv(indio_dev);
> >> + struct regmap *regmap = data->regmap;
> >> + int ret, i, bit;
> >> +
> >> + mutex_lock(>lock);
> >> + switch (scan_mask) {
> >> + case BIT(0) | BIT(1) | BIT(2):
> >> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> >> + mutex_unlock(>lock);
> >> + if (ret < 0)
> >> + goto done;
> >> + break;
> >> + case BIT(0) | BIT(1):
> >> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> >> + mutex_unlock(>lock);
> >> + if (ret < 0)
> >> + goto done;
> >> + break;
> >> + case BIT(1) | BIT(2):
> >> + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> >> + mutex_unlock(>lock);
> >> + if (ret < 0)
> >> + goto done;
> >> + break;
> > What about BIT(0) | BIT(2)?
> >
> > Now you can do it like you have here and on that one corner case let the
> > iio core
> > demux code take care of it, but then you will need to provide
> > available_scan_masks
> > so the core knows it needs to handle this case.
> >
>
> This confused me a little. The available_scan_masks I was using is {BIT(0) |
> BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
> handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2),
> etc.
> Since Phil mentioned he would like this to reduce bus usage as much as we can
> and I want it, too, I think these three circumstances can be read
> consecutively
> while others can be read one axis at a time. So I plan to let BIT(0) |
> BIT(2)
> fall into the 'default' section, which reads axis one by one.
>
> My question is, since this handles every possible combination, do I still
> need
> to list every available scan masks in available_scan_masks?
Ah. I see, I'd missed that the default was picking up that case as well as the
single axes. It would be interesting to sanity check if it is quicker on
a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
and drop the middle value (which would be done using available scan_masks)
or to just do two independent reads.
(I would guess it is worth reading the 'dead' axis).
>
>
> All other problems will be fixed in the next patch.
>
> yours,
>
> Song Qiang
>
>
> ...
Thanks,
Jonathan
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Thu, 18 Oct 2018 16:24:15 +0800
Song Qiang wrote:
> On 2018/10/13 下午6:19, Jonathan Cameron wrote:
> > On Fri, 12 Oct 2018 15:35:36 +0800
> > Song Qiang wrote:
> >
> >> PNI RM3100 is a high resolution, large signal immunity magnetometer,
> >> composed of 3 single sensors and a processing chip with a MagI2C
> >> interface.
> >>
> >> Following functions are available:
> >> - Single-shot measurement from
> >> /sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
> >> - Triggerd buffer measurement.
> >> - DRDY pin for data ready trigger.
> >> - Both i2c and spi interface are supported.
> >> - Both interrupt and polling measurement is supported, depends on if
> >> the 'interrupts' in DT is declared.
> >>
> >> Signed-off-by: Song Qiang
> > A few questions for you (getting very close to being good to go btw!)
> >
> > Why do we have the 3second additional wait for conversions? I know we
> > rarely wait that long, but still seems excessive.
> >
> > Few more comments inline.
> >
> > Thanks,
> >
> > Jonathan
>
> Hi Jonathan,
>
>
> The measurement time of this device varies from 1.7ms to 13 seconds, 3
> seconds
> is just a number in the middle between them. This may be worth discussing,
> hoping to get a better solution from the community.
We should 'know' which of those it will be though as I assume it is dependent
on the device configuration which we control.
So waiting for say, double, the expected time should be sufficient to detect
that things have gone horribly wrong.
>
>
> >> ---
> >> MAINTAINERS| 7 +
> >> drivers/iio/magnetometer/Kconfig | 29 ++
> >> drivers/iio/magnetometer/Makefile | 4 +
> >> drivers/iio/magnetometer/rm3100-core.c | 627 +
> >> drivers/iio/magnetometer/rm3100-i2c.c | 58 +++
> >> drivers/iio/magnetometer/rm3100-spi.c | 64 +++
> >> drivers/iio/magnetometer/rm3100.h | 17 +
> >> 7 files changed, 806 insertions(+)
> >> create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> >> create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> >> create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> >> create mode 100644 drivers/iio/magnetometer/rm3100.h
> >>
>
> ...
>
> >
> >> +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> >> +{
> >> + struct iio_poll_func *pf = p;
> >> + struct iio_dev *indio_dev = pf->indio_dev;
> >> + unsigned long scan_mask = *indio_dev->active_scan_mask;
> >> + unsigned int mask_len = indio_dev->masklength;
> >> + struct rm3100_data *data = iio_priv(indio_dev);
> >> + struct regmap *regmap = data->regmap;
> >> + int ret, i, bit;
> >> +
> >> + mutex_lock(>lock);
> >> + switch (scan_mask) {
> >> + case BIT(0) | BIT(1) | BIT(2):
> >> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> >> + mutex_unlock(>lock);
> >> + if (ret < 0)
> >> + goto done;
> >> + break;
> >> + case BIT(0) | BIT(1):
> >> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> >> + mutex_unlock(>lock);
> >> + if (ret < 0)
> >> + goto done;
> >> + break;
> >> + case BIT(1) | BIT(2):
> >> + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> >> + mutex_unlock(>lock);
> >> + if (ret < 0)
> >> + goto done;
> >> + break;
> > What about BIT(0) | BIT(2)?
> >
> > Now you can do it like you have here and on that one corner case let the
> > iio core
> > demux code take care of it, but then you will need to provide
> > available_scan_masks
> > so the core knows it needs to handle this case.
> >
>
> This confused me a little. The available_scan_masks I was using is {BIT(0) |
> BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
> handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2),
> etc.
> Since Phil mentioned he would like this to reduce bus usage as much as we can
> and I want it, too, I think these three circumstances can be read
> consecutively
> while others can be read one axis at a time. So I plan to let BIT(0) |
> BIT(2)
> fall into the 'default' section, which reads axis one by one.
>
> My question is, since this handles every possible combination, do I still
> need
> to list every available scan masks in available_scan_masks?
Ah. I see, I'd missed that the default was picking up that case as well as the
single axes. It would be interesting to sanity check if it is quicker on
a 'typical' platform to do the all axis read for the BIT(0) | BIT(2) case
and drop the middle value (which would be done using available scan_masks)
or to just do two independent reads.
(I would guess it is worth reading the 'dead' axis).
>
>
> All other problems will be fixed in the next patch.
>
> yours,
>
> Song Qiang
>
>
> ...
Thanks,
Jonathan
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Wed, 17 Oct 2018 16:00:02 +0800
Song Qiang wrote:
> On 2018/10/12 下午8:53, Himanshu Jha wrote:
> > Hi Qiang,
> >
> > On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
> >>
> >> On 2018年10月12日 15:35, Song Qiang wrote:
> >>> PNI RM3100 is a high resolution, large signal immunity magnetometer,
> >>> composed of 3 single sensors and a processing chip with a MagI2C
> >>> interface.
> >>>
> >> ...
> >>> +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> >>> +{
> >>> + struct iio_poll_func *pf = p;
> >>> + struct iio_dev *indio_dev = pf->indio_dev;
> >>> + unsigned long scan_mask = *indio_dev->active_scan_mask;
> >>> + unsigned int mask_len = indio_dev->masklength;
> >>> + struct rm3100_data *data = iio_priv(indio_dev);
> >>> + struct regmap *regmap = data->regmap;
> >>> + int ret, i, bit;
> >>> +
> >>> + mutex_lock(>lock);
> >>> + switch (scan_mask) {
> >>> + case BIT(0) | BIT(1) | BIT(2):
> >>> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> >>> + mutex_unlock(>lock);
> >>> + if (ret < 0)
> >>> + goto done;
> >>> + break;
> >>> + case BIT(0) | BIT(1):
> >>> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> >>> + mutex_unlock(>lock);
> >>> + if (ret < 0)
> >>> + goto done;
> >>> + break;
> >>> + case BIT(1) | BIT(2):
> >>> + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> >>> + mutex_unlock(>lock);
> >>> + if (ret < 0)
> >>> + goto done;
> >>> + break;
> >> Hi Jonathan,
> >>
> >> I just noticed that these three breaks are not proper aligned.
> > Please send the new version of a patch as a *new* thread and don't
> > use `--in-reply-to` flag(if you're using) to chain into older versions
> > as whole thread of older discussion comes up and is often not required.
> >
> > The changelog gives enough info of what's new in the revised series.
> >
> >
> Hi Himanshu,
>
>
> Thanks for your advise.
>
> I did it because the following instruction tells me to, and I think it's
> also a very quick way of gathering
>
> all scattered messages. Both ways have their own advantages and
> disadvantages I think. :)
>
> Section "Updating and
> resending patches".
That's a curious bit of advice. There are certainly a lot of maintainers
who would not want that. It never works with anything beyond trivial
and short patches (we have had patches going to v13 + and hundreds of
emails) - no email client handles that depth and complexity in
a coherent fashion. Replying to previous versions is one of those things
that makes sense until you hit the 'unusual cases' ;)
Oh well. I should probably propose a change to that Doc, but it make
take some time for me to get around to it.
Thanks,
Jonathan
>
>
> yours,
>
> Song Qiang
>
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Wed, 17 Oct 2018 16:00:02 +0800
Song Qiang wrote:
> On 2018/10/12 下午8:53, Himanshu Jha wrote:
> > Hi Qiang,
> >
> > On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
> >>
> >> On 2018年10月12日 15:35, Song Qiang wrote:
> >>> PNI RM3100 is a high resolution, large signal immunity magnetometer,
> >>> composed of 3 single sensors and a processing chip with a MagI2C
> >>> interface.
> >>>
> >> ...
> >>> +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> >>> +{
> >>> + struct iio_poll_func *pf = p;
> >>> + struct iio_dev *indio_dev = pf->indio_dev;
> >>> + unsigned long scan_mask = *indio_dev->active_scan_mask;
> >>> + unsigned int mask_len = indio_dev->masklength;
> >>> + struct rm3100_data *data = iio_priv(indio_dev);
> >>> + struct regmap *regmap = data->regmap;
> >>> + int ret, i, bit;
> >>> +
> >>> + mutex_lock(>lock);
> >>> + switch (scan_mask) {
> >>> + case BIT(0) | BIT(1) | BIT(2):
> >>> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> >>> + mutex_unlock(>lock);
> >>> + if (ret < 0)
> >>> + goto done;
> >>> + break;
> >>> + case BIT(0) | BIT(1):
> >>> + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> >>> + mutex_unlock(>lock);
> >>> + if (ret < 0)
> >>> + goto done;
> >>> + break;
> >>> + case BIT(1) | BIT(2):
> >>> + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> >>> + mutex_unlock(>lock);
> >>> + if (ret < 0)
> >>> + goto done;
> >>> + break;
> >> Hi Jonathan,
> >>
> >> I just noticed that these three breaks are not proper aligned.
> > Please send the new version of a patch as a *new* thread and don't
> > use `--in-reply-to` flag(if you're using) to chain into older versions
> > as whole thread of older discussion comes up and is often not required.
> >
> > The changelog gives enough info of what's new in the revised series.
> >
> >
> Hi Himanshu,
>
>
> Thanks for your advise.
>
> I did it because the following instruction tells me to, and I think it's
> also a very quick way of gathering
>
> all scattered messages. Both ways have their own advantages and
> disadvantages I think. :)
>
> Section "Updating and
> resending patches".
That's a curious bit of advice. There are certainly a lot of maintainers
who would not want that. It never works with anything beyond trivial
and short patches (we have had patches going to v13 + and hundreds of
emails) - no email client handles that depth and complexity in
a coherent fashion. Replying to previous versions is one of those things
that makes sense until you hit the 'unusual cases' ;)
Oh well. I should probably propose a change to that Doc, but it make
take some time for me to get around to it.
Thanks,
Jonathan
>
>
> yours,
>
> Song Qiang
>
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/10/13 下午6:19, Jonathan Cameron wrote:
On Fri, 12 Oct 2018 15:35:36 +0800
Song Qiang wrote:
PNI RM3100 is a high resolution, large signal immunity magnetometer,
composed of 3 single sensors and a processing chip with a MagI2C
interface.
Following functions are available:
- Single-shot measurement from
/sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
- Triggerd buffer measurement.
- DRDY pin for data ready trigger.
- Both i2c and spi interface are supported.
- Both interrupt and polling measurement is supported, depends on if
the 'interrupts' in DT is declared.
Signed-off-by: Song Qiang
A few questions for you (getting very close to being good to go btw!)
Why do we have the 3second additional wait for conversions? I know we
rarely wait that long, but still seems excessive.
Few more comments inline.
Thanks,
Jonathan
Hi Jonathan,
The measurement time of this device varies from 1.7ms to 13 seconds, 3 seconds
is just a number in the middle between them. This may be worth discussing,
hoping to get a better solution from the community.
---
MAINTAINERS| 7 +
drivers/iio/magnetometer/Kconfig | 29 ++
drivers/iio/magnetometer/Makefile | 4 +
drivers/iio/magnetometer/rm3100-core.c | 627 +
drivers/iio/magnetometer/rm3100-i2c.c | 58 +++
drivers/iio/magnetometer/rm3100-spi.c | 64 +++
drivers/iio/magnetometer/rm3100.h | 17 +
7 files changed, 806 insertions(+)
create mode 100644 drivers/iio/magnetometer/rm3100-core.c
create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
create mode 100644 drivers/iio/magnetometer/rm3100.h
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
What about BIT(0) | BIT(2)?
Now you can do it like you have here and on that one corner case let the iio
core
demux code take care of it, but then you will need to provide
available_scan_masks
so the core knows it needs to handle this case.
This confused me a little. The available_scan_masks I was using is {BIT(0) |
BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2), etc.
Since Phil mentioned he would like this to reduce bus usage as much as we can
and I want it, too, I think these three circumstances can be read consecutively
while others can be read one axis at a time. So I plan to let BIT(0) | BIT(2)
fall into the 'default' section, which reads axis one by one.
My question is, since this handles every possible combination, do I still need
to list every available scan masks in available_scan_masks?
All other problems will be fixed in the next patch.
yours,
Song Qiang
...
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/10/13 下午6:19, Jonathan Cameron wrote:
On Fri, 12 Oct 2018 15:35:36 +0800
Song Qiang wrote:
PNI RM3100 is a high resolution, large signal immunity magnetometer,
composed of 3 single sensors and a processing chip with a MagI2C
interface.
Following functions are available:
- Single-shot measurement from
/sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
- Triggerd buffer measurement.
- DRDY pin for data ready trigger.
- Both i2c and spi interface are supported.
- Both interrupt and polling measurement is supported, depends on if
the 'interrupts' in DT is declared.
Signed-off-by: Song Qiang
A few questions for you (getting very close to being good to go btw!)
Why do we have the 3second additional wait for conversions? I know we
rarely wait that long, but still seems excessive.
Few more comments inline.
Thanks,
Jonathan
Hi Jonathan,
The measurement time of this device varies from 1.7ms to 13 seconds, 3 seconds
is just a number in the middle between them. This may be worth discussing,
hoping to get a better solution from the community.
---
MAINTAINERS| 7 +
drivers/iio/magnetometer/Kconfig | 29 ++
drivers/iio/magnetometer/Makefile | 4 +
drivers/iio/magnetometer/rm3100-core.c | 627 +
drivers/iio/magnetometer/rm3100-i2c.c | 58 +++
drivers/iio/magnetometer/rm3100-spi.c | 64 +++
drivers/iio/magnetometer/rm3100.h | 17 +
7 files changed, 806 insertions(+)
create mode 100644 drivers/iio/magnetometer/rm3100-core.c
create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
create mode 100644 drivers/iio/magnetometer/rm3100.h
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
What about BIT(0) | BIT(2)?
Now you can do it like you have here and on that one corner case let the iio
core
demux code take care of it, but then you will need to provide
available_scan_masks
so the core knows it needs to handle this case.
This confused me a little. The available_scan_masks I was using is {BIT(0) |
BIT(1) | BIT(2), 0x0}. Apparently in this version of patch I would like it to
handle every circumstances like BIT(0), BIT(0) | BIT(2), BIT(1) | BIT(2), etc.
Since Phil mentioned he would like this to reduce bus usage as much as we can
and I want it, too, I think these three circumstances can be read consecutively
while others can be read one axis at a time. So I plan to let BIT(0) | BIT(2)
fall into the 'default' section, which reads axis one by one.
My question is, since this handles every possible combination, do I still need
to list every available scan masks in available_scan_masks?
All other problems will be fixed in the next patch.
yours,
Song Qiang
...
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/10/12 下午8:53, Himanshu Jha wrote:
Hi Qiang,
On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
On 2018年10月12日 15:35, Song Qiang wrote:
PNI RM3100 is a high resolution, large signal immunity magnetometer,
composed of 3 single sensors and a processing chip with a MagI2C
interface.
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
Hi Jonathan,
I just noticed that these three breaks are not proper aligned.
Please send the new version of a patch as a *new* thread and don't
use `--in-reply-to` flag(if you're using) to chain into older versions
as whole thread of older discussion comes up and is often not required.
The changelog gives enough info of what's new in the revised series.
Hi Himanshu,
Thanks for your advise.
I did it because the following instruction tells me to, and I think it's
also a very quick way of gathering
all scattered messages. Both ways have their own advantages and
disadvantages I think. :)
Section "Updating and
resending patches".
yours,
Song Qiang
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018/10/12 下午8:53, Himanshu Jha wrote:
Hi Qiang,
On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
On 2018年10月12日 15:35, Song Qiang wrote:
PNI RM3100 is a high resolution, large signal immunity magnetometer,
composed of 3 single sensors and a processing chip with a MagI2C
interface.
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
Hi Jonathan,
I just noticed that these three breaks are not proper aligned.
Please send the new version of a patch as a *new* thread and don't
use `--in-reply-to` flag(if you're using) to chain into older versions
as whole thread of older discussion comes up and is often not required.
The changelog gives enough info of what's new in the revised series.
Hi Himanshu,
Thanks for your advise.
I did it because the following instruction tells me to, and I think it's
also a very quick way of gathering
all scattered messages. Both ways have their own advantages and
disadvantages I think. :)
Section "Updating and
resending patches".
yours,
Song Qiang
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Fri, 12 Oct 2018 15:35:36 +0800
Song Qiang wrote:
> PNI RM3100 is a high resolution, large signal immunity magnetometer,
> composed of 3 single sensors and a processing chip with a MagI2C
> interface.
>
> Following functions are available:
> - Single-shot measurement from
>/sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
> - Triggerd buffer measurement.
> - DRDY pin for data ready trigger.
> - Both i2c and spi interface are supported.
> - Both interrupt and polling measurement is supported, depends on if
>the 'interrupts' in DT is declared.
>
> Signed-off-by: Song Qiang
A few questions for you (getting very close to being good to go btw!)
Why do we have the 3second additional wait for conversions? I know we
rarely wait that long, but still seems excessive.
Few more comments inline.
Thanks,
Jonathan
> ---
> MAINTAINERS| 7 +
> drivers/iio/magnetometer/Kconfig | 29 ++
> drivers/iio/magnetometer/Makefile | 4 +
> drivers/iio/magnetometer/rm3100-core.c | 627 +
> drivers/iio/magnetometer/rm3100-i2c.c | 58 +++
> drivers/iio/magnetometer/rm3100-spi.c | 64 +++
> drivers/iio/magnetometer/rm3100.h | 17 +
> 7 files changed, 806 insertions(+)
> create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> create mode 100644 drivers/iio/magnetometer/rm3100.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 967ce8cdd1cc..14eeeb072403 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11393,6 +11393,13 @@ M: "Rafael J. Wysocki"
> S: Maintained
> F: drivers/pnp/
>
> +PNI RM3100 IIO DRIVER
> +M: Song Qiang
> +L: linux-...@vger.kernel.org
> +S: Maintained
> +F: drivers/iio/magnetometer/rm3100*
> +F: Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> +
> POSIX CLOCKS and TIMERS
> M: Thomas Gleixner
> L: linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/magnetometer/Kconfig
> b/drivers/iio/magnetometer/Kconfig
> index ed9d776d01af..8a63cbbca4b7 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -175,4 +175,33 @@ config SENSORS_HMC5843_SPI
> - hmc5843_core (core functions)
> - hmc5843_spi (support for HMC5983)
>
> +config SENSORS_RM3100
> + tristate
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> +
> +config SENSORS_RM3100_I2C
> + tristate "PNI RM3100 3-Axis Magnetometer (I2C)"
> + depends on I2C
> + select SENSORS_RM3100
> + select REGMAP_I2C
> + help
> + Say Y here to add support for the PNI RM3100 3-Axis Magnetometer.
> +
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called rm3100-i2c.
> +
> +config SENSORS_RM3100_SPI
> + tristate "PNI RM3100 3-Axis Magnetometer (SPI)"
> + depends on SPI_MASTER
> + select SENSORS_RM3100
> + select REGMAP_SPI
> + help
> + Say Y here to add support for the PNI RM3100 3-Axis Magnetometer.
> +
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called rm3100-spi.
> +
> endmenu
> diff --git a/drivers/iio/magnetometer/Makefile
> b/drivers/iio/magnetometer/Makefile
> index 664b2f866472..ba1bc34b82fa 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -24,3 +24,7 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> obj-$(CONFIG_SENSORS_HMC5843)+= hmc5843_core.o
> obj-$(CONFIG_SENSORS_HMC5843_I2C)+= hmc5843_i2c.o
> obj-$(CONFIG_SENSORS_HMC5843_SPI)+= hmc5843_spi.o
> +
> +obj-$(CONFIG_SENSORS_RM3100) += rm3100-core.o
> +obj-$(CONFIG_SENSORS_RM3100_I2C) += rm3100-i2c.o
> +obj-$(CONFIG_SENSORS_RM3100_SPI) += rm3100-spi.o
> diff --git a/drivers/iio/magnetometer/rm3100-core.c
> b/drivers/iio/magnetometer/rm3100-core.c
> new file mode 100644
> index ..d455982ce315
> --- /dev/null
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -0,0 +1,627 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PNI RM3100 3-axis geomagnetic sensor driver core.
> + *
> + * Copyright (C) 2018 Song Qiang
> + *
> + * User Manual available at
> + *
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On Fri, 12 Oct 2018 15:35:36 +0800
Song Qiang wrote:
> PNI RM3100 is a high resolution, large signal immunity magnetometer,
> composed of 3 single sensors and a processing chip with a MagI2C
> interface.
>
> Following functions are available:
> - Single-shot measurement from
>/sys/bus/iio/devices/iio:deviceX/in_magn_{axis}_raw
> - Triggerd buffer measurement.
> - DRDY pin for data ready trigger.
> - Both i2c and spi interface are supported.
> - Both interrupt and polling measurement is supported, depends on if
>the 'interrupts' in DT is declared.
>
> Signed-off-by: Song Qiang
A few questions for you (getting very close to being good to go btw!)
Why do we have the 3second additional wait for conversions? I know we
rarely wait that long, but still seems excessive.
Few more comments inline.
Thanks,
Jonathan
> ---
> MAINTAINERS| 7 +
> drivers/iio/magnetometer/Kconfig | 29 ++
> drivers/iio/magnetometer/Makefile | 4 +
> drivers/iio/magnetometer/rm3100-core.c | 627 +
> drivers/iio/magnetometer/rm3100-i2c.c | 58 +++
> drivers/iio/magnetometer/rm3100-spi.c | 64 +++
> drivers/iio/magnetometer/rm3100.h | 17 +
> 7 files changed, 806 insertions(+)
> create mode 100644 drivers/iio/magnetometer/rm3100-core.c
> create mode 100644 drivers/iio/magnetometer/rm3100-i2c.c
> create mode 100644 drivers/iio/magnetometer/rm3100-spi.c
> create mode 100644 drivers/iio/magnetometer/rm3100.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 967ce8cdd1cc..14eeeb072403 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11393,6 +11393,13 @@ M: "Rafael J. Wysocki"
> S: Maintained
> F: drivers/pnp/
>
> +PNI RM3100 IIO DRIVER
> +M: Song Qiang
> +L: linux-...@vger.kernel.org
> +S: Maintained
> +F: drivers/iio/magnetometer/rm3100*
> +F: Documentation/devicetree/bindings/iio/magnetometer/pni,rm3100.txt
> +
> POSIX CLOCKS and TIMERS
> M: Thomas Gleixner
> L: linux-kernel@vger.kernel.org
> diff --git a/drivers/iio/magnetometer/Kconfig
> b/drivers/iio/magnetometer/Kconfig
> index ed9d776d01af..8a63cbbca4b7 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -175,4 +175,33 @@ config SENSORS_HMC5843_SPI
> - hmc5843_core (core functions)
> - hmc5843_spi (support for HMC5983)
>
> +config SENSORS_RM3100
> + tristate
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> +
> +config SENSORS_RM3100_I2C
> + tristate "PNI RM3100 3-Axis Magnetometer (I2C)"
> + depends on I2C
> + select SENSORS_RM3100
> + select REGMAP_I2C
> + help
> + Say Y here to add support for the PNI RM3100 3-Axis Magnetometer.
> +
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called rm3100-i2c.
> +
> +config SENSORS_RM3100_SPI
> + tristate "PNI RM3100 3-Axis Magnetometer (SPI)"
> + depends on SPI_MASTER
> + select SENSORS_RM3100
> + select REGMAP_SPI
> + help
> + Say Y here to add support for the PNI RM3100 3-Axis Magnetometer.
> +
> + This driver can also be compiled as a module.
> + To compile this driver as a module, choose M here: the module
> + will be called rm3100-spi.
> +
> endmenu
> diff --git a/drivers/iio/magnetometer/Makefile
> b/drivers/iio/magnetometer/Makefile
> index 664b2f866472..ba1bc34b82fa 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -24,3 +24,7 @@ obj-$(CONFIG_IIO_ST_MAGN_SPI_3AXIS) += st_magn_spi.o
> obj-$(CONFIG_SENSORS_HMC5843)+= hmc5843_core.o
> obj-$(CONFIG_SENSORS_HMC5843_I2C)+= hmc5843_i2c.o
> obj-$(CONFIG_SENSORS_HMC5843_SPI)+= hmc5843_spi.o
> +
> +obj-$(CONFIG_SENSORS_RM3100) += rm3100-core.o
> +obj-$(CONFIG_SENSORS_RM3100_I2C) += rm3100-i2c.o
> +obj-$(CONFIG_SENSORS_RM3100_SPI) += rm3100-spi.o
> diff --git a/drivers/iio/magnetometer/rm3100-core.c
> b/drivers/iio/magnetometer/rm3100-core.c
> new file mode 100644
> index ..d455982ce315
> --- /dev/null
> +++ b/drivers/iio/magnetometer/rm3100-core.c
> @@ -0,0 +1,627 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * PNI RM3100 3-axis geomagnetic sensor driver core.
> + *
> + * Copyright (C) 2018 Song Qiang
> + *
> + * User Manual available at
> + *
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
Hi Qiang,
On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
>
>
> On 2018年10月12日 15:35, Song Qiang wrote:
> > PNI RM3100 is a high resolution, large signal immunity magnetometer,
> > composed of 3 single sensors and a processing chip with a MagI2C
> > interface.
> >
> ...
> > +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + unsigned long scan_mask = *indio_dev->active_scan_mask;
> > + unsigned int mask_len = indio_dev->masklength;
> > + struct rm3100_data *data = iio_priv(indio_dev);
> > + struct regmap *regmap = data->regmap;
> > + int ret, i, bit;
> > +
> > + mutex_lock(>lock);
> > + switch (scan_mask) {
> > + case BIT(0) | BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(0) | BIT(1):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> Hi Jonathan,
>
> I just noticed that these three breaks are not proper aligned.
Please send the new version of a patch as a *new* thread and don't
use `--in-reply-to` flag(if you're using) to chain into older versions
as whole thread of older discussion comes up and is often not required.
The changelog gives enough info of what's new in the revised series.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
Hi Qiang,
On Fri, Oct 12, 2018 at 04:36:01PM +0800, Song Qiang wrote:
>
>
> On 2018年10月12日 15:35, Song Qiang wrote:
> > PNI RM3100 is a high resolution, large signal immunity magnetometer,
> > composed of 3 single sensors and a processing chip with a MagI2C
> > interface.
> >
> ...
> > +static irqreturn_t rm3100_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + unsigned long scan_mask = *indio_dev->active_scan_mask;
> > + unsigned int mask_len = indio_dev->masklength;
> > + struct rm3100_data *data = iio_priv(indio_dev);
> > + struct regmap *regmap = data->regmap;
> > + int ret, i, bit;
> > +
> > + mutex_lock(>lock);
> > + switch (scan_mask) {
> > + case BIT(0) | BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(0) | BIT(1):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> > + case BIT(1) | BIT(2):
> > + ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
> > + mutex_unlock(>lock);
> > + if (ret < 0)
> > + goto done;
> > + break;
> Hi Jonathan,
>
> I just noticed that these three breaks are not proper aligned.
Please send the new version of a patch as a *new* thread and don't
use `--in-reply-to` flag(if you're using) to chain into older versions
as whole thread of older discussion comes up and is often not required.
The changelog gives enough info of what's new in the revised series.
--
Himanshu Jha
Undergraduate Student
Department of Electronics & Communication
Guru Tegh Bahadur Institute of Technology
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018年10月12日 15:35, Song Qiang wrote:
PNI RM3100 is a high resolution, large signal immunity magnetometer,
composed of 3 single sensors and a processing chip with a MagI2C
interface.
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
Hi Jonathan,
I just noticed that these three breaks are not proper aligned.
yours,
Song Qiang
Re: [PATCH v4 3/3] iio: magnetometer: Add driver support for PNI RM3100
On 2018年10月12日 15:35, Song Qiang wrote:
PNI RM3100 is a high resolution, large signal immunity magnetometer,
composed of 3 single sensors and a processing chip with a MagI2C
interface.
...
+static irqreturn_t rm3100_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ unsigned long scan_mask = *indio_dev->active_scan_mask;
+ unsigned int mask_len = indio_dev->masklength;
+ struct rm3100_data *data = iio_priv(indio_dev);
+ struct regmap *regmap = data->regmap;
+ int ret, i, bit;
+
+ mutex_lock(>lock);
+ switch (scan_mask) {
+ case BIT(0) | BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 9);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(0) | BIT(1):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MX2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
+ case BIT(1) | BIT(2):
+ ret = regmap_bulk_read(regmap, RM3100_REG_MY2, data->buffer, 6);
+ mutex_unlock(>lock);
+ if (ret < 0)
+ goto done;
+ break;
Hi Jonathan,
I just noticed that these three breaks are not proper aligned.
yours,
Song Qiang
