On Wed, Mar 17, 2021 at 6:38 AM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Wed, Mar 17, 2021 at 3:21 PM Alexandru Ardelean
> <ardeleanalex@xxxxxxxxx> wrote:
> > On Wed, Mar 17, 2021 at 3:16 PM Andy Shevchenko
> > <andy.shevchenko@xxxxxxxxx> wrote:
> > >
> > > On Tue, Mar 16, 2021 at 11:24 PM Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:
> > > >
> > > > fixes commit 2e2366c2d141 ("iio: cros_ec: unify hw fifo attributes into the core file")
> > > > fixes commit 165aea80e2e2 ("iio: cros_ec: use devm_iio_triggered_buffer_setup_ext()")
> > >
> > > Use the proper Fixes tag(s) in the tag block below.
> > >
> > > > 1. Instead of adding has_fw_fifo, deduct it from the configuration:
> > > > - EC must support FIFO (EC_FEATURE_MOTION_SENSE_FIFO) set.
> > > > - sensors send data a regular interval (accelerometer, gyro,
> > > > magnetomer, barometer, light sensor).
> > > > - "Legacy accelerometer" is only present on EC without FIFO, so we don't
> > > > need to set buffer attributes.
> > > >
> > > > 2. devm_iio_triggered_buffer_setup_ext() does not need to be called when
> > > > EC does not support FIFO, as there is no FIFO to manage.
> > > >
> > > > 3. Use devm_iio_triggered_buffer_setup_ext() when EC has a FIFO to
> > > > specify the buffer extended attributes.
> > >
> > > Sounds like three patches in one. Please, split and add proper Fixes
> > > tag(s) to each of them.
> >
> > The code [at least] can be a single patch.
> > Albeit, it describes 3 different issues.
>
> I didn't get this. Is it one fix in one place which luckily fixes
> three issues (because the root cause of them is the same) or it's
> really three fixes in one patch because of some other reasons?
> In either case first what should be done is elaboration in the commit
> message(s). And if the latter is true, this needs to be split to patch
> per (logical) fix.
As it stands, we can not properly read sensor samples from the
chromebook embedded controller (EC).
The commits 2e2366c2d141 and 165aea80e2e2 should be reverted, but the
kernel would not compile as the IIO API has changed
(iio_buffer_set_attrs() has been removed in commit 21232b4456b ("iio:
buffer: remove iio_buffer_set_attrs() helper")). It would be a problem
when we do bisections.
This single commit removes the unnecessary code and set the buffer
|attr| field at the right place, reverting back to a working cros_ec
driver.
I thought about reverting commits 21232b4456b, 165aea80e2e2,
2e2366c2d141, adding a commit to call
devm_iio_triggered_buffer_setup_ext() instead of
iio_buffer_set_attrs(), and then reapplying 21232b4456b, but I believe
it is more confusing than a single commit.
Gwendal.
>
> --
> With Best Regards,
> Andy Shevchenko
