| Message ID | 20220210224933.379149-5-yury.norov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | None | expand |
On Thu, Feb 10, 2022 at 02:48:48PM -0800, Yury Norov wrote: > iio_simple_dummy_trigger_h() is mostly an opencoded for_each_set_bit(). > Using for_each_set_bit() make code much cleaner, and more effective. I would wait for some testing, but from code perspective looks good. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Yury Norov <yury.norov@gmail.com> > --- > drivers/iio/dummy/iio_simple_dummy_buffer.c | 48 ++++++++------------- > 1 file changed, 19 insertions(+), 29 deletions(-) > > diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c > index d81c2b2dad82..3bc1b7529e2a 100644 > --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c > +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c > @@ -45,41 +45,31 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) > { > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > + int i = 0, j; > u16 *data; > > data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > if (!data) > goto done; > > - if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) { > - /* > - * Three common options here: > - * hardware scans: certain combinations of channels make > - * up a fast read. The capture will consist of all of them. > - * Hence we just call the grab data function and fill the > - * buffer without processing. > - * software scans: can be considered to be random access > - * so efficient reading is just a case of minimal bus > - * transactions. > - * software culled hardware scans: > - * occasionally a driver may process the nearest hardware > - * scan to avoid storing elements that are not desired. This > - * is the fiddliest option by far. > - * Here let's pretend we have random access. And the values are > - * in the constant table fakedata. > - */ > - int i, j; > - > - for (i = 0, j = 0; > - i < bitmap_weight(indio_dev->active_scan_mask, > - indio_dev->masklength); > - i++, j++) { > - j = find_next_bit(indio_dev->active_scan_mask, > - indio_dev->masklength, j); > - /* random access read from the 'device' */ > - data[i] = fakedata[j]; > - } > - } > + /* > + * Three common options here: > + * hardware scans: certain combinations of channels make > + * up a fast read. The capture will consist of all of them. > + * Hence we just call the grab data function and fill the > + * buffer without processing. > + * software scans: can be considered to be random access > + * so efficient reading is just a case of minimal bus > + * transactions. > + * software culled hardware scans: > + * occasionally a driver may process the nearest hardware > + * scan to avoid storing elements that are not desired. This > + * is the fiddliest option by far. > + * Here let's pretend we have random access. And the values are > + * in the constant table fakedata. > + */ > + for_each_set_bit(j, indio_dev->active_scan_mask, indio_dev->masklength) > + data[i++] = fakedata[j]; > > iio_push_to_buffers_with_timestamp(indio_dev, data, > iio_get_time_ns(indio_dev)); > -- > 2.32.0 >
Le 10/02/2022 à 23:48, Yury Norov a écrit : > iio_simple_dummy_trigger_h() is mostly an opencoded for_each_set_bit(). > Using for_each_set_bit() make code much cleaner, and more effective. > > Signed-off-by: Yury Norov <yury.norov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > --- > drivers/iio/dummy/iio_simple_dummy_buffer.c | 48 ++++++++------------- > 1 file changed, 19 insertions(+), 29 deletions(-) > > diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c > index d81c2b2dad82..3bc1b7529e2a 100644 > --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c > +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c > @@ -45,41 +45,31 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) > { > struct iio_poll_func *pf = p; > struct iio_dev *indio_dev = pf->indio_dev; > + int i = 0, j; > u16 *data; > > data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > if (!data) > goto done; > > - if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) { > - /* > - * Three common options here: > - * hardware scans: certain combinations of channels make > - * up a fast read. The capture will consist of all of them. > - * Hence we just call the grab data function and fill the > - * buffer without processing. > - * software scans: can be considered to be random access > - * so efficient reading is just a case of minimal bus > - * transactions. > - * software culled hardware scans: > - * occasionally a driver may process the nearest hardware > - * scan to avoid storing elements that are not desired. This > - * is the fiddliest option by far. > - * Here let's pretend we have random access. And the values are > - * in the constant table fakedata. > - */ > - int i, j; > - > - for (i = 0, j = 0; > - i < bitmap_weight(indio_dev->active_scan_mask, > - indio_dev->masklength); > - i++, j++) { > - j = find_next_bit(indio_dev->active_scan_mask, > - indio_dev->masklength, j); > - /* random access read from the 'device' */ > - data[i] = fakedata[j]; > - } > - } > + /* > + * Three common options here: > + * hardware scans: certain combinations of channels make > + * up a fast read. The capture will consist of all of them. > + * Hence we just call the grab data function and fill the > + * buffer without processing. > + * software scans: can be considered to be random access > + * so efficient reading is just a case of minimal bus > + * transactions. > + * software culled hardware scans: > + * occasionally a driver may process the nearest hardware > + * scan to avoid storing elements that are not desired. This > + * is the fiddliest option by far. > + * Here let's pretend we have random access. And the values are > + * in the constant table fakedata. > + */ Nitpicking: you could take advantage of the tab you save to use the full width of the line and save some lines of code. Just my 2c. CJ > + for_each_set_bit(j, indio_dev->active_scan_mask, indio_dev->masklength) > + data[i++] = fakedata[j]; > > iio_push_to_buffers_with_timestamp(indio_dev, data, > iio_get_time_ns(indio_dev));
On Fri, 11 Feb 2022 18:17:37 +0100 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > Le 10/02/2022 à 23:48, Yury Norov a écrit : > > iio_simple_dummy_trigger_h() is mostly an opencoded for_each_set_bit(). > > Using for_each_set_bit() make code much cleaner, and more effective. > > > > Signed-off-by: Yury Norov <yury.norov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > --- > > drivers/iio/dummy/iio_simple_dummy_buffer.c | 48 ++++++++------------- > > 1 file changed, 19 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c > > index d81c2b2dad82..3bc1b7529e2a 100644 > > --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c > > +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c > > @@ -45,41 +45,31 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) > > { > > struct iio_poll_func *pf = p; > > struct iio_dev *indio_dev = pf->indio_dev; > > + int i = 0, j; > > u16 *data; > > > > data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > > if (!data) > > goto done; > > > > - if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) { > > - /* > > - * Three common options here: > > - * hardware scans: certain combinations of channels make > > - * up a fast read. The capture will consist of all of them. > > - * Hence we just call the grab data function and fill the > > - * buffer without processing. > > - * software scans: can be considered to be random access > > - * so efficient reading is just a case of minimal bus > > - * transactions. > > - * software culled hardware scans: > > - * occasionally a driver may process the nearest hardware > > - * scan to avoid storing elements that are not desired. This > > - * is the fiddliest option by far. > > - * Here let's pretend we have random access. And the values are > > - * in the constant table fakedata. > > - */ > > - int i, j; > > - > > - for (i = 0, j = 0; > > - i < bitmap_weight(indio_dev->active_scan_mask, > > - indio_dev->masklength); > > - i++, j++) { > > - j = find_next_bit(indio_dev->active_scan_mask, > > - indio_dev->masklength, j); > > - /* random access read from the 'device' */ > > - data[i] = fakedata[j]; > > - } > > - } > > + /* > > + * Three common options here: > > + * hardware scans: certain combinations of channels make > > + * up a fast read. The capture will consist of all of them. > > + * Hence we just call the grab data function and fill the > > + * buffer without processing. > > + * software scans: can be considered to be random access > > + * so efficient reading is just a case of minimal bus > > + * transactions. > > + * software culled hardware scans: > > + * occasionally a driver may process the nearest hardware > > + * scan to avoid storing elements that are not desired. This > > + * is the fiddliest option by far. > > + * Here let's pretend we have random access. And the values are > > + * in the constant table fakedata. > > + */ > > Nitpicking: you could take advantage of the tab you save to use the full > width of the line and save some lines of code. Tweaked whilst applying. Sorry this one took so long. I marked it as a patch that I'd revisit if and tidy up if there was no v2 sent, but then managed to forget about it until I came to do a clean out of patchwork today. Anyhow, now applied to the togreg branch of iio.git - initially pushed out as testing for 0-day to see if we missed anything. Thanks, Jonathan > > Just my 2c. > > CJ > > > > + for_each_set_bit(j, indio_dev->active_scan_mask, indio_dev->masklength) > > + data[i++] = fakedata[j]; > > > > iio_push_to_buffers_with_timestamp(indio_dev, data, > > iio_get_time_ns(indio_dev)); >
On Sat, 4 Jun 2022 16:41:13 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 11 Feb 2022 18:17:37 +0100 > Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote: > > > Le 10/02/2022 à 23:48, Yury Norov a écrit : > > > iio_simple_dummy_trigger_h() is mostly an opencoded for_each_set_bit(). > > > Using for_each_set_bit() make code much cleaner, and more effective. > > > > > > Signed-off-by: Yury Norov <yury.norov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > > > --- > > > drivers/iio/dummy/iio_simple_dummy_buffer.c | 48 ++++++++------------- > > > 1 file changed, 19 insertions(+), 29 deletions(-) > > > > > > diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c > > > index d81c2b2dad82..3bc1b7529e2a 100644 > > > --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c > > > +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c > > > @@ -45,41 +45,31 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) > > > { > > > struct iio_poll_func *pf = p; > > > struct iio_dev *indio_dev = pf->indio_dev; > > > + int i = 0, j; > > > u16 *data; > > > > > > data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); > > > if (!data) > > > goto done; > > > > > > - if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) { > > > - /* > > > - * Three common options here: > > > - * hardware scans: certain combinations of channels make > > > - * up a fast read. The capture will consist of all of them. > > > - * Hence we just call the grab data function and fill the > > > - * buffer without processing. > > > - * software scans: can be considered to be random access > > > - * so efficient reading is just a case of minimal bus > > > - * transactions. > > > - * software culled hardware scans: > > > - * occasionally a driver may process the nearest hardware > > > - * scan to avoid storing elements that are not desired. This > > > - * is the fiddliest option by far. > > > - * Here let's pretend we have random access. And the values are > > > - * in the constant table fakedata. > > > - */ > > > - int i, j; > > > - > > > - for (i = 0, j = 0; > > > - i < bitmap_weight(indio_dev->active_scan_mask, > > > - indio_dev->masklength); > > > - i++, j++) { > > > - j = find_next_bit(indio_dev->active_scan_mask, > > > - indio_dev->masklength, j); > > > - /* random access read from the 'device' */ > > > - data[i] = fakedata[j]; > > > - } > > > - } > > > + /* > > > + * Three common options here: > > > + * hardware scans: certain combinations of channels make > > > + * up a fast read. The capture will consist of all of them. > > > + * Hence we just call the grab data function and fill the > > > + * buffer without processing. > > > + * software scans: can be considered to be random access > > > + * so efficient reading is just a case of minimal bus > > > + * transactions. > > > + * software culled hardware scans: > > > + * occasionally a driver may process the nearest hardware > > > + * scan to avoid storing elements that are not desired. This > > > + * is the fiddliest option by far. > > > + * Here let's pretend we have random access. And the values are > > > + * in the constant table fakedata. > > > + */ > > > > Nitpicking: you could take advantage of the tab you save to use the full > > width of the line and save some lines of code. > > Tweaked whilst applying. > > Sorry this one took so long. I marked it as a patch that I'd revisit if and > tidy up if there was no v2 sent, but then managed to forget about it until > I came to do a clean out of patchwork today. > > Anyhow, now applied to the togreg branch of iio.git - initially pushed out > as testing for 0-day to see if we missed anything. And dropped again during a rebase as a different version has gone upstream through a pull request to Linus. Whilst I have no strong opinion on that in general, I am a little grumpy that a version was merged that was never posted to the mailing lists (that I can find on lore.kernel.org.) Sure the changes were minor and easy to verify as harmless, but none the less they should have been posted. Jonathan > > Thanks, > > Jonathan > > > > > Just my 2c. > > > > CJ > > > > > > > + for_each_set_bit(j, indio_dev->active_scan_mask, indio_dev->masklength) > > > + data[i++] = fakedata[j]; > > > > > > iio_push_to_buffers_with_timestamp(indio_dev, data, > > > iio_get_time_ns(indio_dev)); > > >
diff --git a/drivers/iio/dummy/iio_simple_dummy_buffer.c b/drivers/iio/dummy/iio_simple_dummy_buffer.c index d81c2b2dad82..3bc1b7529e2a 100644 --- a/drivers/iio/dummy/iio_simple_dummy_buffer.c +++ b/drivers/iio/dummy/iio_simple_dummy_buffer.c @@ -45,41 +45,31 @@ static irqreturn_t iio_simple_dummy_trigger_h(int irq, void *p) { struct iio_poll_func *pf = p; struct iio_dev *indio_dev = pf->indio_dev; + int i = 0, j; u16 *data; data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL); if (!data) goto done; - if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) { - /* - * Three common options here: - * hardware scans: certain combinations of channels make - * up a fast read. The capture will consist of all of them. - * Hence we just call the grab data function and fill the - * buffer without processing. - * software scans: can be considered to be random access - * so efficient reading is just a case of minimal bus - * transactions. - * software culled hardware scans: - * occasionally a driver may process the nearest hardware - * scan to avoid storing elements that are not desired. This - * is the fiddliest option by far. - * Here let's pretend we have random access. And the values are - * in the constant table fakedata. - */ - int i, j; - - for (i = 0, j = 0; - i < bitmap_weight(indio_dev->active_scan_mask, - indio_dev->masklength); - i++, j++) { - j = find_next_bit(indio_dev->active_scan_mask, - indio_dev->masklength, j); - /* random access read from the 'device' */ - data[i] = fakedata[j]; - } - } + /* + * Three common options here: + * hardware scans: certain combinations of channels make + * up a fast read. The capture will consist of all of them. + * Hence we just call the grab data function and fill the + * buffer without processing. + * software scans: can be considered to be random access + * so efficient reading is just a case of minimal bus + * transactions. + * software culled hardware scans: + * occasionally a driver may process the nearest hardware + * scan to avoid storing elements that are not desired. This + * is the fiddliest option by far. + * Here let's pretend we have random access. And the values are + * in the constant table fakedata. + */ + for_each_set_bit(j, indio_dev->active_scan_mask, indio_dev->masklength) + data[i++] = fakedata[j]; iio_push_to_buffers_with_timestamp(indio_dev, data, iio_get_time_ns(indio_dev));
iio_simple_dummy_trigger_h() is mostly an opencoded for_each_set_bit(). Using for_each_set_bit() make code much cleaner, and more effective. Signed-off-by: Yury Norov <yury.norov@gmail.com> --- drivers/iio/dummy/iio_simple_dummy_buffer.c | 48 ++++++++------------- 1 file changed, 19 insertions(+), 29 deletions(-)