| Message ID | 20200929125949.69934-8-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: remove iio_buffer_set_attrs() and assign buffer attrs during alloc | expand |
On Tue, Sep 29, 2020 at 3:55 PM Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This change switches to the new devm_iio_triggered_buffer_setup_ext() > function and removes the iio_buffer_set_attrs() call, for assigning the > HW FIFO attributes to the buffer. Sorry, you were too fast with the version, below one nit. > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > --- > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > index c62cacc04672..1eafcf04ad69 100644 > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > if (ret) > return ret; > } else { > + const struct attribute **fifo_attrs; > + > + if (has_hw_fifo) > + fifo_attrs = cros_ec_sensor_fifo_attributes; > + else > + fifo_attrs = NULL; > + > /* > * The only way to get samples in buffer is to set a > * software trigger (systrig, hrtimer). > */ > - ret = devm_iio_triggered_buffer_setup( > + ret = devm_iio_triggered_buffer_setup_ext( > dev, indio_dev, NULL, trigger_capture, > - NULL); > + NULL, fifo_attrs); Perhaps it's time to reformat a bit, i.e. move dev to the first line and do the rest accordingly? > if (ret) > return ret; > - > - if (has_hw_fifo) > - iio_buffer_set_attrs(indio_dev->buffer, > - cros_ec_sensor_fifo_attributes); > } > } > > -- > 2.17.1 >
On Tue, Sep 29, 2020 at 4:09 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Tue, Sep 29, 2020 at 3:55 PM Alexandru Ardelean > <alexandru.ardelean@analog.com> wrote: > > > This change switches to the new devm_iio_triggered_buffer_setup_ext() > > function and removes the iio_buffer_set_attrs() call, for assigning the > > HW FIFO attributes to the buffer. > > Sorry, you were too fast with the version, below one nit. > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > --- > > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > index c62cacc04672..1eafcf04ad69 100644 > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > if (ret) > > return ret; > > } else { > > + const struct attribute **fifo_attrs; > > + > > + if (has_hw_fifo) > > + fifo_attrs = cros_ec_sensor_fifo_attributes; > > + else > > + fifo_attrs = NULL; > > + > > /* > > * The only way to get samples in buffer is to set a > > * software trigger (systrig, hrtimer). > > */ > > - ret = devm_iio_triggered_buffer_setup( > > > + ret = devm_iio_triggered_buffer_setup_ext( > > dev, indio_dev, NULL, trigger_capture, > > - NULL); > > + NULL, fifo_attrs); > > Perhaps it's time to reformat a bit, i.e. move dev to the first line > and do the rest accordingly? this feels like a mix of preferences here; for once, the patch here [as-is], is the minimal form for this change [in terms of patch-noise]; so, some people would choose the least noisiest patch; also, this indentation was chosen [as-is here] from the start [for this code block]; not sure if it was preferred; i'd suspect it was due to the old 80-col limit; i'd leave it as-is [for now], or defer the decision to a maintainer to decide [either IIO or chromium]; > > > if (ret) > > return ret; > > - > > - if (has_hw_fifo) > > - iio_buffer_set_attrs(indio_dev->buffer, > > - cros_ec_sensor_fifo_attributes); > > } > > } > > > > -- > > 2.17.1 > > > > > -- > With Best Regards, > Andy Shevchenko
On Tue, 29 Sep 2020 17:31:55 +0300 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Tue, Sep 29, 2020 at 4:09 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Tue, Sep 29, 2020 at 3:55 PM Alexandru Ardelean > > <alexandru.ardelean@analog.com> wrote: > > > > > This change switches to the new devm_iio_triggered_buffer_setup_ext() > > > function and removes the iio_buffer_set_attrs() call, for assigning the > > > HW FIFO attributes to the buffer. > > > > Sorry, you were too fast with the version, below one nit. > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > --- > > > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > index c62cacc04672..1eafcf04ad69 100644 > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > > if (ret) > > > return ret; > > > } else { > > > + const struct attribute **fifo_attrs; > > > + > > > + if (has_hw_fifo) > > > + fifo_attrs = cros_ec_sensor_fifo_attributes; > > > + else > > > + fifo_attrs = NULL; > > > + > > > /* > > > * The only way to get samples in buffer is to set a > > > * software trigger (systrig, hrtimer). > > > */ > > > - ret = devm_iio_triggered_buffer_setup( > > > > > + ret = devm_iio_triggered_buffer_setup_ext( > > > dev, indio_dev, NULL, trigger_capture, > > > - NULL); > > > + NULL, fifo_attrs); > > > > Perhaps it's time to reformat a bit, i.e. move dev to the first line > > and do the rest accordingly? > > this feels like a mix of preferences here; > for once, the patch here [as-is], is the minimal form for this change > [in terms of patch-noise]; > so, some people would choose the least noisiest patch; > > also, this indentation was chosen [as-is here] from the start [for > this code block]; > not sure if it was preferred; i'd suspect it was due to the old 80-col limit; > > i'd leave it as-is [for now], or defer the decision to a maintainer to > decide [either IIO or chromium]; The indenting of this whole code block is a bit too deep. Looks to me like we should flip the sense of the outer if statement if (!physical_device) return 0; That would lead to a whole bunch of reformatting around here including picking up this. For now I can just shuffle it a bit whilst applying. This set isn't likely to make the merge window anyway now as I'd like it to sit on list a little longer just because it touches several drivers with active maintainers and I'd like time for them to sanity check. Jonathan > > > > > > if (ret) > > > return ret; > > > - > > > - if (has_hw_fifo) > > > - iio_buffer_set_attrs(indio_dev->buffer, > > > - cros_ec_sensor_fifo_attributes); > > > } > > > } > > > > > > -- > > > 2.17.1 > > > > > > > > > -- > > With Best Regards, > > Andy Shevchenko
On Tue, Sep 29, 2020 at 6:40 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 29 Sep 2020 17:31:55 +0300 > Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > > > On Tue, Sep 29, 2020 at 4:09 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > > > > On Tue, Sep 29, 2020 at 3:55 PM Alexandru Ardelean > > > <alexandru.ardelean@analog.com> wrote: > > > > > > > This change switches to the new devm_iio_triggered_buffer_setup_ext() > > > > function and removes the iio_buffer_set_attrs() call, for assigning the > > > > HW FIFO attributes to the buffer. > > > > > > Sorry, you were too fast with the version, below one nit. > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > --- > > > > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > index c62cacc04672..1eafcf04ad69 100644 > > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > > > if (ret) > > > > return ret; > > > > } else { > > > > + const struct attribute **fifo_attrs; > > > > + > > > > + if (has_hw_fifo) > > > > + fifo_attrs = cros_ec_sensor_fifo_attributes; > > > > + else > > > > + fifo_attrs = NULL; > > > > + > > > > /* > > > > * The only way to get samples in buffer is to set a > > > > * software trigger (systrig, hrtimer). > > > > */ > > > > - ret = devm_iio_triggered_buffer_setup( > > > > > > > + ret = devm_iio_triggered_buffer_setup_ext( > > > > dev, indio_dev, NULL, trigger_capture, > > > > - NULL); > > > > + NULL, fifo_attrs); > > > > > > Perhaps it's time to reformat a bit, i.e. move dev to the first line > > > and do the rest accordingly? > > > > this feels like a mix of preferences here; > > for once, the patch here [as-is], is the minimal form for this change > > [in terms of patch-noise]; > > so, some people would choose the least noisiest patch; > > > > also, this indentation was chosen [as-is here] from the start [for > > this code block]; > > not sure if it was preferred; i'd suspect it was due to the old 80-col limit; > > > > i'd leave it as-is [for now], or defer the decision to a maintainer to > > decide [either IIO or chromium]; > > The indenting of this whole code block is a bit too deep. > > Looks to me like we should flip the sense of the outer if statement > > if (!physical_device) > return 0; > > That would lead to a whole bunch of reformatting around here including > picking up this. > > For now I can just shuffle it a bit whilst applying. > > This set isn't likely to make the merge window anyway now as I'd like > it to sit on list a little longer just because it touches several > drivers with active maintainers and I'd like time for them to sanity > check. > ping on this; should i do a V4 for this? this is related to the multiple IIO buffer support: https://lore.kernel.org/linux-iio/20201117162340.43924-1-alexandru.ardelean@analog.com/T/#t it's one of the patchsets i could split away on it's own; > Jonathan > > > > > > > > > > > if (ret) > > > > return ret; > > > > - > > > > - if (has_hw_fifo) > > > > - iio_buffer_set_attrs(indio_dev->buffer, > > > > - cros_ec_sensor_fifo_attributes); > > > > } > > > > } > > > > > > > > -- > > > > 2.17.1 > > > > > > > > > > > > > -- > > > With Best Regards, > > > Andy Shevchenko >
On Wed, 18 Nov 2020 12:35:16 +0200 Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > On Tue, Sep 29, 2020 at 6:40 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Tue, 29 Sep 2020 17:31:55 +0300 > > Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > > > > > On Tue, Sep 29, 2020 at 4:09 PM Andy Shevchenko > > > <andy.shevchenko@gmail.com> wrote: > > > > > > > > On Tue, Sep 29, 2020 at 3:55 PM Alexandru Ardelean > > > > <alexandru.ardelean@analog.com> wrote: > > > > > > > > > This change switches to the new devm_iio_triggered_buffer_setup_ext() > > > > > function and removes the iio_buffer_set_attrs() call, for assigning the > > > > > HW FIFO attributes to the buffer. > > > > > > > > Sorry, you were too fast with the version, below one nit. > > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > > --- > > > > > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > > index c62cacc04672..1eafcf04ad69 100644 > > > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > > @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > > > > if (ret) > > > > > return ret; > > > > > } else { > > > > > + const struct attribute **fifo_attrs; > > > > > + > > > > > + if (has_hw_fifo) > > > > > + fifo_attrs = cros_ec_sensor_fifo_attributes; > > > > > + else > > > > > + fifo_attrs = NULL; > > > > > + > > > > > /* > > > > > * The only way to get samples in buffer is to set a > > > > > * software trigger (systrig, hrtimer). > > > > > */ > > > > > - ret = devm_iio_triggered_buffer_setup( > > > > > > > > > + ret = devm_iio_triggered_buffer_setup_ext( > > > > > dev, indio_dev, NULL, trigger_capture, > > > > > - NULL); > > > > > + NULL, fifo_attrs); > > > > > > > > Perhaps it's time to reformat a bit, i.e. move dev to the first line > > > > and do the rest accordingly? > > > > > > this feels like a mix of preferences here; > > > for once, the patch here [as-is], is the minimal form for this change > > > [in terms of patch-noise]; > > > so, some people would choose the least noisiest patch; > > > > > > also, this indentation was chosen [as-is here] from the start [for > > > this code block]; > > > not sure if it was preferred; i'd suspect it was due to the old 80-col limit; > > > > > > i'd leave it as-is [for now], or defer the decision to a maintainer to > > > decide [either IIO or chromium]; > > > > The indenting of this whole code block is a bit too deep. > > > > Looks to me like we should flip the sense of the outer if statement > > > > if (!physical_device) > > return 0; > > > > That would lead to a whole bunch of reformatting around here including > > picking up this. > > > > For now I can just shuffle it a bit whilst applying. > > > > This set isn't likely to make the merge window anyway now as I'd like > > it to sit on list a little longer just because it touches several > > drivers with active maintainers and I'd like time for them to sanity > > check. > > > > ping on this; > should i do a V4 for this? Yes, probably worth sending out again. I'd like to see a few more acks on the individual drivers ideally and a v4 will get this to the top of peoples' inboxes. If we don't get them I won't let it block this series, but it's nice to try at least! Thanks, Jonathan > > this is related to the multiple IIO buffer support: > https://lore.kernel.org/linux-iio/20201117162340.43924-1-alexandru.ardelean@analog.com/T/#t > > it's one of the patchsets i could split away on it's own; > > > Jonathan > > > > > > > > > > > > > > > > if (ret) > > > > > return ret; > > > > > - > > > > > - if (has_hw_fifo) > > > > > - iio_buffer_set_attrs(indio_dev->buffer, > > > > > - cros_ec_sensor_fifo_attributes); > > > > > } > > > > > } > > > > > > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > > > > > > > -- > > > > With Best Regards, > > > > Andy Shevchenko > >
On Sat, 21 Nov 2020 14:47:39 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Wed, 18 Nov 2020 12:35:16 +0200 > Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > > > On Tue, Sep 29, 2020 at 6:40 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Tue, 29 Sep 2020 17:31:55 +0300 > > > Alexandru Ardelean <ardeleanalex@gmail.com> wrote: > > > > > > > On Tue, Sep 29, 2020 at 4:09 PM Andy Shevchenko > > > > <andy.shevchenko@gmail.com> wrote: > > > > > > > > > > On Tue, Sep 29, 2020 at 3:55 PM Alexandru Ardelean > > > > > <alexandru.ardelean@analog.com> wrote: > > > > > > > > > > > This change switches to the new devm_iio_triggered_buffer_setup_ext() > > > > > > function and removes the iio_buffer_set_attrs() call, for assigning the > > > > > > HW FIFO attributes to the buffer. > > > > > > > > > > Sorry, you were too fast with the version, below one nit. > > > > > > > > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > > > > > --- > > > > > > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ > > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > > > diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > > > index c62cacc04672..1eafcf04ad69 100644 > > > > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c > > > > > > @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > > > > > if (ret) > > > > > > return ret; > > > > > > } else { > > > > > > + const struct attribute **fifo_attrs; > > > > > > + > > > > > > + if (has_hw_fifo) > > > > > > + fifo_attrs = cros_ec_sensor_fifo_attributes; > > > > > > + else > > > > > > + fifo_attrs = NULL; > > > > > > + > > > > > > /* > > > > > > * The only way to get samples in buffer is to set a > > > > > > * software trigger (systrig, hrtimer). > > > > > > */ > > > > > > - ret = devm_iio_triggered_buffer_setup( > > > > > > > > > > > + ret = devm_iio_triggered_buffer_setup_ext( > > > > > > dev, indio_dev, NULL, trigger_capture, > > > > > > - NULL); > > > > > > + NULL, fifo_attrs); > > > > > > > > > > Perhaps it's time to reformat a bit, i.e. move dev to the first line > > > > > and do the rest accordingly? > > > > > > > > this feels like a mix of preferences here; > > > > for once, the patch here [as-is], is the minimal form for this change > > > > [in terms of patch-noise]; > > > > so, some people would choose the least noisiest patch; > > > > > > > > also, this indentation was chosen [as-is here] from the start [for > > > > this code block]; > > > > not sure if it was preferred; i'd suspect it was due to the old 80-col limit; > > > > > > > > i'd leave it as-is [for now], or defer the decision to a maintainer to > > > > decide [either IIO or chromium]; > > > > > > The indenting of this whole code block is a bit too deep. > > > > > > Looks to me like we should flip the sense of the outer if statement > > > > > > if (!physical_device) > > > return 0; > > > > > > That would lead to a whole bunch of reformatting around here including > > > picking up this. > > > > > > For now I can just shuffle it a bit whilst applying. > > > > > > This set isn't likely to make the merge window anyway now as I'd like > > > it to sit on list a little longer just because it touches several > > > drivers with active maintainers and I'd like time for them to sanity > > > check. > > > > > > > ping on this; > > should i do a V4 for this? > Yes, probably worth sending out again. I'd like to see a few more acks > on the individual drivers ideally and a v4 will get this to the > top of peoples' inboxes. > > If we don't get them I won't let it block this series, but it's nice > to try at least! Actually scratch that this is old now and should be fine to check by inspection. I'll have a go at applying it and if nothing odd happens we should be good. I was too lazy to fix the alignment Andy pointed out but would like to see the more major refactoring as discussed for that patch if one of us gets round to it sometime :) Hmm. b4 doesn't deal with accidental half typed email addresses so just went through and fixed all those. Thanks, Jonathan > > Thanks, > > Jonathan > > > > > this is related to the multiple IIO buffer support: > > https://lore.kernel.org/linux-iio/20201117162340.43924-1-alexandru.ardelean@analog.com/T/#t > > > > it's one of the patchsets i could split away on it's own; > > > > > Jonathan > > > > > > > > > > > > > > > > > > > > > if (ret) > > > > > > return ret; > > > > > > - > > > > > > - if (has_hw_fifo) > > > > > > - iio_buffer_set_attrs(indio_dev->buffer, > > > > > > - cros_ec_sensor_fifo_attributes); > > > > > > } > > > > > > } > > > > > > > > > > > > -- > > > > > > 2.17.1 > > > > > > > > > > > > > > > > > > > > > -- > > > > > With Best Regards, > > > > > Andy Shevchenko > > > >
diff --git a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c index c62cacc04672..1eafcf04ad69 100644 --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c @@ -353,19 +353,22 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, if (ret) return ret; } else { + const struct attribute **fifo_attrs; + + if (has_hw_fifo) + fifo_attrs = cros_ec_sensor_fifo_attributes; + else + fifo_attrs = NULL; + /* * The only way to get samples in buffer is to set a * software trigger (systrig, hrtimer). */ - ret = devm_iio_triggered_buffer_setup( + ret = devm_iio_triggered_buffer_setup_ext( dev, indio_dev, NULL, trigger_capture, - NULL); + NULL, fifo_attrs); if (ret) return ret; - - if (has_hw_fifo) - iio_buffer_set_attrs(indio_dev->buffer, - cros_ec_sensor_fifo_attributes); } }
This change switches to the new devm_iio_triggered_buffer_setup_ext() function and removes the iio_buffer_set_attrs() call, for assigning the HW FIFO attributes to the buffer. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)