Re: [PATCH v2 2/2] iio: cros_ec : Extend legacy support to ARM device

[Gwendal Grignou]

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 = {                                          \