On Sat, Jun 22, 2019 at 2:03 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Thu, 20 Jun 2019 19:41:06 -0700
> Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote:
>
> > Add support to ARM based devices, that lack LPC access code.
> > Allow cros-ec-accel-legacy to use cros-ec-sensors-core, add specific
> > command to capture sensor data.
> >
> > On veyron minnie, check chrome detect tablet mode and rotate
> > screen in tablet mode.
> > Check only a subset of the attributes are presented.
>
> Superficially this looks like 2 changes to me. One moves
> over to using the shared code. The other sounds like it's introducing
> the new device support?
That's correct.
> 2 patches perhaps?
Will do.
>
> However, looking at the code, am I interpreting the description
> correctly? Looks to me like we are really just reducing duplicated
> code by using the cros_ec_sensors_core code to replace
> existing code in the cros-ec-accel-legacy driver? If that's
> the main focus, then make that clear in the patch title.
The second part introduce a command to retrieve sensor data that is
applicable for veyron-minnie only, cros_ec_accel_legacy_read_cmd.
>
> One passing comment inline.
>
> Thanks,
>
> Jonathan
>
>
> >
> > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > ---
> > drivers/iio/accel/Kconfig | 4 +-
> > drivers/iio/accel/cros_ec_accel_legacy.c | 350 +++++------------------
> > 2 files changed, 79 insertions(+), 275 deletions(-)
> >
> > diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
> > index 62a970a20219..7d0848f9ea45 100644
> > --- a/drivers/iio/accel/Kconfig
> > +++ b/drivers/iio/accel/Kconfig
> > @@ -201,9 +201,7 @@ config HID_SENSOR_ACCEL_3D
> >
> > config IIO_CROS_EC_ACCEL_LEGACY
> > tristate "ChromeOS EC Legacy Accelerometer Sensor"
> > - select IIO_BUFFER
> > - select IIO_TRIGGERED_BUFFER
> > - select CROS_EC_LPC_REGISTER_DEVICE
> > + depends on IIO_CROS_EC_SENSORS_CORE
> > help
> > Say yes here to get support for accelerometers on Chromebook using
> > legacy EC firmware.
> > diff --git a/drivers/iio/accel/cros_ec_accel_legacy.c b/drivers/iio/accel/cros_ec_accel_legacy.c
> > index 46bb2e421bb9..575d7e4c685c 100644
> > --- a/drivers/iio/accel/cros_ec_accel_legacy.c
> > +++ b/drivers/iio/accel/cros_ec_accel_legacy.c
> > @@ -5,13 +5,14 @@
> > * Copyright 2017 Google, Inc
> > *
> > * This driver uses the memory mapper cros-ec interface to communicate
> > - * with the Chrome OS EC about accelerometer data.
> > + * with the Chrome OS EC about accelerometer data or older commands.
...
> > #define CROS_EC_ACCEL_LEGACY_CHAN(_axis) \
> > { \
> > @@ -319,31 +140,30 @@ static const struct iio_chan_spec_ext_info cros_ec_accel_legacy_ext_info[] = {
> > .modified = 1, \
> > .info_mask_separate = \
> > BIT(IIO_CHAN_INFO_RAW) | \
> > - BIT(IIO_CHAN_INFO_SCALE) | \
>
> My first thought was you were changing the ABI, but looking closer this
> was broken anyway as we shouldn't have had both separate and shared by all
> for the same thing on the same channel. Still presumably an ABI change
> but one fixing a broken interface so fingers crossed no one notices.
Yes, it was an error that was not documented in the ABI. I will fix
that separately.
>
> > BIT(IIO_CHAN_INFO_CALIBBIAS), \
> > .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SCALE), \
> > - .ext_info = cros_ec_accel_legacy_ext_info, \
> > + .ext_info = cros_ec_sensors_ext_info, \
> > .scan_type = { \
