| Message ID | 20220817105643.95710-4-contact@artur-rojek.eu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio/adc-joystick: buffer data parsing fixes | expand |
On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> wrote: > > This is useful for consumers that wish to parse raw buffer data. ... > +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + struct iio_buffer *buffer) > +{ > + int length, offset = 0; > + unsigned int si; > + > + if (chan->scan_index < 0 || > + !test_bit(chan->scan_index, buffer->scan_mask)) { > + return -EINVAL; > + } Have you run checkpatch? The {} are redundant. But personally I would split this into two separate conditionals. > + for (si = 0; si < chan->scan_index; ++si) { Just a side crying: where did you, people, get this pre-increment pattern from?! > + if (!test_bit(si, buffer->scan_mask)) > + continue; NIH for_each_set_bit() > + length = iio_storage_bytes_for_si(indio_dev, si); > + > + /* Account for channel alignment. */ > + if (offset % length) > + offset += length - (offset % length); > + offset += length; > + } > + > + return offset; > +} > +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer); Same Q as per previous patch: IIO namespace?
On 2022-08-19 10:17, Andy Shevchenko wrote: > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> > wrote: >> >> This is useful for consumers that wish to parse raw buffer data. > > ... > >> +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev, >> + const struct iio_chan_spec >> *chan, >> + struct iio_buffer *buffer) >> +{ >> + int length, offset = 0; >> + unsigned int si; >> + >> + if (chan->scan_index < 0 || >> + !test_bit(chan->scan_index, buffer->scan_mask)) { >> + return -EINVAL; >> + } > > Have you run checkpatch? The {} are redundant. But personally I would > split this into two separate conditionals. I did run checkpatch on it - all patches were ready for submission. I don't find the {} redundant for multi-line statements, like this one, and I personally prefer to check conditions that return the same error type together. > >> + for (si = 0; si < chan->scan_index; ++si) { > > Just a side crying: where did you, people, get this pre-increment > pattern from?! > >> + if (!test_bit(si, buffer->scan_mask)) >> + continue; > > NIH for_each_set_bit() > >> + length = iio_storage_bytes_for_si(indio_dev, si); >> + >> + /* Account for channel alignment. */ >> + if (offset % length) >> + offset += length - (offset % length); >> + offset += length; >> + } >> + >> + return offset; >> +} >> +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer); > > Same Q as per previous patch: IIO namespace?
On Fri, Aug 19, 2022 at 1:33 PM Artur Rojek <contact@artur-rojek.eu> wrote: > On 2022-08-19 10:17, Andy Shevchenko wrote: > > On Wed, Aug 17, 2022 at 1:58 PM Artur Rojek <contact@artur-rojek.eu> > > wrote: ... > >> + if (chan->scan_index < 0 || > >> + !test_bit(chan->scan_index, buffer->scan_mask)) { > >> + return -EINVAL; > >> + } > > > > Have you run checkpatch? The {} are redundant. But personally I would > > split this into two separate conditionals. > I did run checkpatch on it - all patches were ready for submission. > I don't find the {} redundant for multi-line statements, like this one, This is a one-line conditional. So, *unlike* this one. > and I personally prefer to check conditions that return the same error > type together. I see that the maintainer's input is needed here, because even if the error code is the same, the semantics are different and I consider that to prevail on the combining.
On Wed, 17 Aug 2022 12:56:42 +0200 Artur Rojek <contact@artur-rojek.eu> wrote: > This is useful for consumers that wish to parse raw buffer data. > > Tested-by: Paul Cercueil <paul@crapouillou.net> > Signed-off-by: Artur Rojek <contact@artur-rojek.eu> > --- > drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++++++++++++ > include/linux/iio/buffer.h | 4 ++++ > 2 files changed, 32 insertions(+) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index 228598b82a2f..cf23736610d9 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -691,6 +691,34 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev, > return bytes; > } > > +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + struct iio_buffer *buffer) > +{ > + int length, offset = 0; > + unsigned int si; > + > + if (chan->scan_index < 0 || > + !test_bit(chan->scan_index, buffer->scan_mask)) { > + return -EINVAL; > + } > + > + for (si = 0; si < chan->scan_index; ++si) { > + if (!test_bit(si, buffer->scan_mask)) You are walking channels that the consumer should not even know about here. I think you can establish the same easily enough from the channels it does know about. It would be fiddly if you had lots of channels (as you might be best off sorting them) but for small numbers of channels loop over the array to find lowest scan_index - compute offset due to that then move on to next one etc until you reach the scan index of the channel you want. > + continue; > + > + length = iio_storage_bytes_for_si(indio_dev, si); > + > + /* Account for channel alignment. */ > + if (offset % length) > + offset += length - (offset % length); > + offset += length; > + } > + > + return offset; > +} > +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer); > + > static unsigned int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev) > { > struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); > diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h > index 418b1307d3f2..b1db74772e77 100644 > --- a/include/linux/iio/buffer.h > +++ b/include/linux/iio/buffer.h > @@ -16,6 +16,10 @@ enum iio_buffer_direction { > IIO_BUFFER_DIRECTION_OUT, > }; > > +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + struct iio_buffer *buffer); > + > int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data); > > int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index 228598b82a2f..cf23736610d9 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -691,6 +691,34 @@ static unsigned int iio_storage_bytes_for_si(struct iio_dev *indio_dev, return bytes; } +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + struct iio_buffer *buffer) +{ + int length, offset = 0; + unsigned int si; + + if (chan->scan_index < 0 || + !test_bit(chan->scan_index, buffer->scan_mask)) { + return -EINVAL; + } + + for (si = 0; si < chan->scan_index; ++si) { + if (!test_bit(si, buffer->scan_mask)) + continue; + + length = iio_storage_bytes_for_si(indio_dev, si); + + /* Account for channel alignment. */ + if (offset % length) + offset += length - (offset % length); + offset += length; + } + + return offset; +} +EXPORT_SYMBOL_GPL(iio_find_channel_offset_in_buffer); + static unsigned int iio_storage_bytes_for_timestamp(struct iio_dev *indio_dev) { struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(indio_dev); diff --git a/include/linux/iio/buffer.h b/include/linux/iio/buffer.h index 418b1307d3f2..b1db74772e77 100644 --- a/include/linux/iio/buffer.h +++ b/include/linux/iio/buffer.h @@ -16,6 +16,10 @@ enum iio_buffer_direction { IIO_BUFFER_DIRECTION_OUT, }; +int iio_find_channel_offset_in_buffer(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + struct iio_buffer *buffer); + int iio_push_to_buffers(struct iio_dev *indio_dev, const void *data); int iio_pop_from_buffer(struct iio_buffer *buffer, void *data);