On Wed, Mar 17, 2021 at 7:11 PM Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:
>
> 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.
Please, elaborate this in the commit message and use proper Fixes tags.
--
With Best Regards,
Andy Shevchenko
