| Message ID | 20200917125951.861-1-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: buffer: split buffer sysfs creation to take buffer as primary arg | expand |
On Thu, 17 Sep 2020 15:59:51 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as > primary argument. This change splits the main logic into a private function > that takes an IIO buffer as primary argument. > > That way, the functions can be extended to configure the sysfs for multiple > buffers. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> One comment inline. Whilst I think it is safe as you have it, I'd rather avoid the minor change in logic if we don't need to make it. Thanks, Jonathan > --- > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++----------- > 1 file changed, 30 insertions(+), 16 deletions(-) > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > index a7d7e5143ed2..a4f6bb96d4f4 100644 > --- a/drivers/iio/industrialio-buffer.c > +++ b/drivers/iio/industrialio-buffer.c > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = { > &dev_attr_data_available.attr, > }; > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > + struct iio_dev *indio_dev) > { > struct iio_dev_attr *p; > struct attribute **attr; > - struct iio_buffer *buffer = indio_dev->buffer; > int ret, i, attrn, attrcount; > const struct iio_chan_spec *channels; > > - channels = indio_dev->channels; > - if (channels) { > - int ml = indio_dev->masklength; > - > - for (i = 0; i < indio_dev->num_channels; i++) > - ml = max(ml, channels[i].scan_index + 1); > - indio_dev->masklength = ml; > - } > - > - if (!buffer) > - return 0; > - > attrcount = 0; > if (buffer->attrs) { > while (buffer->attrs[attrcount] != NULL) > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > return ret; > } > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > { > struct iio_buffer *buffer = indio_dev->buffer; > + const struct iio_chan_spec *channels; > + int i; > + > + channels = indio_dev->channels; > + if (channels) { > + int ml = indio_dev->masklength; > + > + for (i = 0; i < indio_dev->num_channels; i++) > + ml = max(ml, channels[i].scan_index + 1); > + indio_dev->masklength = ml; > + } I've not really figured out if it matters, but this is a logic change. Previously we didn't compute masklength if there was no buffer provided. Now we do. It's probably better to move the if (!buffer) check above this block or at least mention this change in the patch description. > > if (!buffer) > - return; > + return 0; > + > + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev); > +} > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > +{ > bitmap_free(buffer->scan_mask); > kfree(buffer->buffer_group.attrs); > kfree(buffer->scan_el_group.attrs); > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > } > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > +{ > + struct iio_buffer *buffer = indio_dev->buffer; > + > + if (!buffer) > + return; > + > + __iio_buffer_free_sysfs_and_mask(buffer); > +} > + > /** > * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected > * @indio_dev: the iio device
On Thu, Sep 17, 2020 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 17 Sep 2020 15:59:51 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as > > primary argument. This change splits the main logic into a private function > > that takes an IIO buffer as primary argument. > > > > That way, the functions can be extended to configure the sysfs for multiple > > buffers. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > One comment inline. Whilst I think it is safe as you have it, I'd > rather avoid the minor change in logic if we don't need to make it. > > Thanks, > > Jonathan > > > > --- > > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++----------- > > 1 file changed, 30 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > > index a7d7e5143ed2..a4f6bb96d4f4 100644 > > --- a/drivers/iio/industrialio-buffer.c > > +++ b/drivers/iio/industrialio-buffer.c > > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = { > > &dev_attr_data_available.attr, > > }; > > > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > + struct iio_dev *indio_dev) > > { > > struct iio_dev_attr *p; > > struct attribute **attr; > > - struct iio_buffer *buffer = indio_dev->buffer; > > int ret, i, attrn, attrcount; > > const struct iio_chan_spec *channels; > > > > - channels = indio_dev->channels; > > - if (channels) { > > - int ml = indio_dev->masklength; > > - > > - for (i = 0; i < indio_dev->num_channels; i++) > > - ml = max(ml, channels[i].scan_index + 1); > > - indio_dev->masklength = ml; > > - } > > - > > - if (!buffer) > > - return 0; > > - > > attrcount = 0; > > if (buffer->attrs) { > > while (buffer->attrs[attrcount] != NULL) > > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > return ret; > > } > > > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > { > > struct iio_buffer *buffer = indio_dev->buffer; > > + const struct iio_chan_spec *channels; > > + int i; > > + > > + channels = indio_dev->channels; > > + if (channels) { > > + int ml = indio_dev->masklength; > > + > > + for (i = 0; i < indio_dev->num_channels; i++) > > + ml = max(ml, channels[i].scan_index + 1); > > + indio_dev->masklength = ml; > > + } > > I've not really figured out if it matters, but this is a logic change. > Previously we didn't compute masklength if there was no buffer provided. > Now we do. It's probably better to move the if (!buffer) check above > this block or at least mention this change in the patch description. > Umm, are you referring that this patch is a logic change or you are suggesting a logic change? The "if (!buffer)" check was positioned after the masklength computation even in the old code. > > > > > if (!buffer) > > - return; > > + return 0; > > + > > + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev); > > +} > > > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > > +{ > > bitmap_free(buffer->scan_mask); > > kfree(buffer->buffer_group.attrs); > > kfree(buffer->scan_el_group.attrs); > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > } > > > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > +{ > > + struct iio_buffer *buffer = indio_dev->buffer; > > + > > + if (!buffer) > > + return; > > + > > + __iio_buffer_free_sysfs_and_mask(buffer); > > +} > > + > > /** > > * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected > > * @indio_dev: the iio device >
On Thu, 17 Sep 2020 20:41:08 +0300 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Thu, Sep 17, 2020 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Thu, 17 Sep 2020 15:59:51 +0300 > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as > > > primary argument. This change splits the main logic into a private function > > > that takes an IIO buffer as primary argument. > > > > > > That way, the functions can be extended to configure the sysfs for multiple > > > buffers. > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > One comment inline. Whilst I think it is safe as you have it, I'd > > rather avoid the minor change in logic if we don't need to make it. > > > > Thanks, > > > > Jonathan Applied to the togreg branch of iio.git. See below for my pathetic Diff confused me excuse :) Jonathan > > > > > > > --- > > > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++----------- > > > 1 file changed, 30 insertions(+), 16 deletions(-) > > > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > > > index a7d7e5143ed2..a4f6bb96d4f4 100644 > > > --- a/drivers/iio/industrialio-buffer.c > > > +++ b/drivers/iio/industrialio-buffer.c > > > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = { > > > &dev_attr_data_available.attr, > > > }; > > > > > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > > + struct iio_dev *indio_dev) > > > { > > > struct iio_dev_attr *p; > > > struct attribute **attr; > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > int ret, i, attrn, attrcount; > > > const struct iio_chan_spec *channels; > > > > > > - channels = indio_dev->channels; > > > - if (channels) { > > > - int ml = indio_dev->masklength; > > > - > > > - for (i = 0; i < indio_dev->num_channels; i++) > > > - ml = max(ml, channels[i].scan_index + 1); > > > - indio_dev->masklength = ml; > > > - } > > > - > > > - if (!buffer) > > > - return 0; > > > - > > > attrcount = 0; > > > if (buffer->attrs) { > > > while (buffer->attrs[attrcount] != NULL) > > > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > > return ret; > > > } > > > > > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > > { > > > struct iio_buffer *buffer = indio_dev->buffer; > > > + const struct iio_chan_spec *channels; > > > + int i; > > > + > > > + channels = indio_dev->channels; > > > + if (channels) { > > > + int ml = indio_dev->masklength; > > > + > > > + for (i = 0; i < indio_dev->num_channels; i++) > > > + ml = max(ml, channels[i].scan_index + 1); > > > + indio_dev->masklength = ml; > > > + } > > > > I've not really figured out if it matters, but this is a logic change. > > Previously we didn't compute masklength if there was no buffer provided. > > Now we do. It's probably better to move the if (!buffer) check above > > this block or at least mention this change in the patch description. > > > > Umm, are you referring that this patch is a logic change or you are > suggesting a logic change? > The "if (!buffer)" check was positioned after the masklength > computation even in the old code. > Got you. Diff confused me :) > > > > > > > > if (!buffer) > > > - return; > > > + return 0; > > > + > > > + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev); > > > +} > > > > > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > > > +{ > > > bitmap_free(buffer->scan_mask); > > > kfree(buffer->buffer_group.attrs); > > > kfree(buffer->scan_el_group.attrs); > > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > > } > > > > > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > > +{ > > > + struct iio_buffer *buffer = indio_dev->buffer; > > > + > > > + if (!buffer) > > > + return; > > > + > > > + __iio_buffer_free_sysfs_and_mask(buffer); > > > +} > > > + > > > /** > > > * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected > > > * @indio_dev: the iio device > >
On Thu, Sep 17, 2020 at 8:56 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 17 Sep 2020 20:41:08 +0300 > Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > > > On Thu, Sep 17, 2020 at 8:18 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Thu, 17 Sep 2020 15:59:51 +0300 > > > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > > > > > Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as > > > > primary argument. This change splits the main logic into a private function > > > > that takes an IIO buffer as primary argument. > > > > > > > > That way, the functions can be extended to configure the sysfs for multiple > > > > buffers. > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > > > One comment inline. Whilst I think it is safe as you have it, I'd > > > rather avoid the minor change in logic if we don't need to make it. > > > > > > Thanks, > > > > > > Jonathan > Applied to the togreg branch of iio.git. > > See below for my pathetic Diff confused me excuse :) > > Jonathan > > > > > > > > > > > --- > > > > drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++----------- > > > > 1 file changed, 30 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c > > > > index a7d7e5143ed2..a4f6bb96d4f4 100644 > > > > --- a/drivers/iio/industrialio-buffer.c > > > > +++ b/drivers/iio/industrialio-buffer.c > > > > @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = { > > > > &dev_attr_data_available.attr, > > > > }; > > > > > > > > -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > > > +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, > > > > + struct iio_dev *indio_dev) > > > > { > > > > struct iio_dev_attr *p; > > > > struct attribute **attr; > > > > - struct iio_buffer *buffer = indio_dev->buffer; > > > > int ret, i, attrn, attrcount; > > > > const struct iio_chan_spec *channels; > > > > > > > > - channels = indio_dev->channels; > > > > - if (channels) { > > > > - int ml = indio_dev->masklength; > > > > - > > > > - for (i = 0; i < indio_dev->num_channels; i++) > > > > - ml = max(ml, channels[i].scan_index + 1); > > > > - indio_dev->masklength = ml; > > > > - } > > > > - > > > > - if (!buffer) > > > > - return 0; > > > > - > > > > attrcount = 0; > > > > if (buffer->attrs) { > > > > while (buffer->attrs[attrcount] != NULL) > > > > @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > > > return ret; > > > > } > > > > > > > > -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > > > +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) > > > > { > > > > struct iio_buffer *buffer = indio_dev->buffer; > > > > + const struct iio_chan_spec *channels; > > > > + int i; > > > > + > > > > + channels = indio_dev->channels; > > > > + if (channels) { > > > > + int ml = indio_dev->masklength; > > > > + > > > > + for (i = 0; i < indio_dev->num_channels; i++) > > > > + ml = max(ml, channels[i].scan_index + 1); > > > > + indio_dev->masklength = ml; > > > > + } > > > > > > I've not really figured out if it matters, but this is a logic change. > > > Previously we didn't compute masklength if there was no buffer provided. > > > Now we do. It's probably better to move the if (!buffer) check above > > > this block or at least mention this change in the patch description. > > > > > > > Umm, are you referring that this patch is a logic change or you are > > suggesting a logic change? > > The "if (!buffer)" check was positioned after the masklength > > computation even in the old code. > > > Got you. Diff confused me :) Yeah. Happens to me to with some git diffs. > > > > > > > > > > > > > if (!buffer) > > > > - return; > > > > + return 0; > > > > + > > > > + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev); > > > > +} > > > > > > > > +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) > > > > +{ > > > > bitmap_free(buffer->scan_mask); > > > > kfree(buffer->buffer_group.attrs); > > > > kfree(buffer->scan_el_group.attrs); > > > > iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); > > > > } > > > > > > > > +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) > > > > +{ > > > > + struct iio_buffer *buffer = indio_dev->buffer; > > > > + > > > > + if (!buffer) > > > > + return; > > > > + > > > > + __iio_buffer_free_sysfs_and_mask(buffer); > > > > +} > > > > + > > > > /** > > > > * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected > > > > * @indio_dev: the iio device > > > >
diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c index a7d7e5143ed2..a4f6bb96d4f4 100644 --- a/drivers/iio/industrialio-buffer.c +++ b/drivers/iio/industrialio-buffer.c @@ -1264,26 +1264,14 @@ static struct attribute *iio_buffer_attrs[] = { &dev_attr_data_available.attr, }; -int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) +static int __iio_buffer_alloc_sysfs_and_mask(struct iio_buffer *buffer, + struct iio_dev *indio_dev) { struct iio_dev_attr *p; struct attribute **attr; - struct iio_buffer *buffer = indio_dev->buffer; int ret, i, attrn, attrcount; const struct iio_chan_spec *channels; - channels = indio_dev->channels; - if (channels) { - int ml = indio_dev->masklength; - - for (i = 0; i < indio_dev->num_channels; i++) - ml = max(ml, channels[i].scan_index + 1); - indio_dev->masklength = ml; - } - - if (!buffer) - return 0; - attrcount = 0; if (buffer->attrs) { while (buffer->attrs[attrcount] != NULL) @@ -1367,19 +1355,45 @@ int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) return ret; } -void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) +int iio_buffer_alloc_sysfs_and_mask(struct iio_dev *indio_dev) { struct iio_buffer *buffer = indio_dev->buffer; + const struct iio_chan_spec *channels; + int i; + + channels = indio_dev->channels; + if (channels) { + int ml = indio_dev->masklength; + + for (i = 0; i < indio_dev->num_channels; i++) + ml = max(ml, channels[i].scan_index + 1); + indio_dev->masklength = ml; + } if (!buffer) - return; + return 0; + + return __iio_buffer_alloc_sysfs_and_mask(buffer, indio_dev); +} +static void __iio_buffer_free_sysfs_and_mask(struct iio_buffer *buffer) +{ bitmap_free(buffer->scan_mask); kfree(buffer->buffer_group.attrs); kfree(buffer->scan_el_group.attrs); iio_free_chan_devattr_list(&buffer->scan_el_dev_attr_list); } +void iio_buffer_free_sysfs_and_mask(struct iio_dev *indio_dev) +{ + struct iio_buffer *buffer = indio_dev->buffer; + + if (!buffer) + return; + + __iio_buffer_free_sysfs_and_mask(buffer); +} + /** * iio_validate_scan_mask_onehot() - Validates that exactly one channel is selected * @indio_dev: the iio device
Currently the iio_buffer_{alloc,free}_sysfs_and_mask() take 'indio_dev' as primary argument. This change splits the main logic into a private function that takes an IIO buffer as primary argument. That way, the functions can be extended to configure the sysfs for multiple buffers. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-buffer.c | 46 ++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 16 deletions(-)