Re: [PATCH] iio: buffer: split buffer sysfs creation to take buffer as primary arg

[Alexandru Ardelean <ardeleanalex@gmail.com>]

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
> > >
>