Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice

[Lee Jones <lee.jones@linaro.org>]

On Thu, 30 May 2019, Gwendal Grignou wrote:

> On Thu, May 30, 2019 at 12:48 AM Lee Jones <lee.jones@linaro.org> wrote:
> >
> > On Wed, 29 May 2019, Gwendal Grignou wrote:
> >
> > > On Wed, May 29, 2019 at 4:44 AM Lee Jones <lee.jones@linaro.org> wrote:
> > > >
> > > > On Tue, 28 May 2019, Gwendal Grignou wrote:
> > > >
> > > > > On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@linaro.org> wrote:
> > > > > >
> > > > > > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> > > > > >
> > > > > > > From: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > >
> > > > > > > With this patch, the cros_ec_ctl driver will register the legacy
> > > > > > > accelerometer driver (named cros_ec_accel_legacy) if it fails to
> > > > > > > register sensors through the usual path cros_ec_sensors_register().
> > > > > > > This legacy device is present on Chromebook devices with older EC
> > > > > > > firmware only supporting deprecated EC commands (Glimmer based devices).
> > > > > > >
> > > > > > > Tested-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > > > > > > Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
> > > > > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > > > > ---
> > > > > > > Changes in v5:
> > > > > > > - Remove unnecessary white lines.
> > > > > > >
> > > > > > > Changes in v4:
> > > > > > > - [5/8] Nit: EC -> ECs (Lee Jones)
> > > > > > > - [5/8] Statically define cros_ec_accel_legacy_cells (Lee Jones)
> > > > > > >
> > > > > > > Changes in v3:
> > > > > > > - [5/8] Add the Reviewed-by Andy Shevchenko.
> > > > > > >
> > > > > > > Changes in v2:
> > > > > > > - [5/8] Add the Reviewed-by Gwendal.
> > > > > > >
> > > > > > >  drivers/mfd/cros_ec_dev.c | 66 +++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 66 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > > > > > > index d275deaecb12..64567bd0a081 100644
> > > > > > > --- a/drivers/mfd/cros_ec_dev.c
> > > > > > > +++ b/drivers/mfd/cros_ec_dev.c
> > > > > > > @@ -376,6 +376,69 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> > > > > > >       kfree(msg);
> > > > > > >  }
> > > > > > >
> > > > > > > +static struct cros_ec_sensor_platform sensor_platforms[] = {
> > > > > > > +     { .sensor_num = 0 },
> > > > > > > +     { .sensor_num = 1 }
> > > > > > > +};
> > > > > >
> > > > > > I'm still very uncomfortable with this struct.
> > > > > >
> > > > > > Other than these indices, the sensors have no other distinguishing
> > > > > > features, thus there should be no need to identify or distinguish
> > > > > > between them in this way.
> > > > > When initializing the sensors, the IIO driver expect to find in the
> > > > > data  structure pointed by dev_get_platdata(dev), in field sensor_num
> > > > > is stored the index assigned by the embedded controller to talk to a
> > > > > given sensor.
> > > > > cros_ec_sensors_register() use the same mechanism; in that function,
> > > > > the sensor_num field is populated from the output of an EC command
> > > > > MOTIONSENSE_CMD_INFO. In case of legacy mode, that command may not be
> > > > > available and in any case we know the EC has only either 2
> > > > > accelerometers present or nothing.
> > > > >
> > > > > For instance, let's compare a legacy device with a more recent one:
> > > > >
> > > > > legacy:
> > > > > type                  |   id          | sensor_num   | device name
> > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > >
> > > > > Modern:
> > > > > type                  |   id          | sensor_num   | device name
> > > > > accelerometer  |   0           |   0                  | cros-ec-accel.0
> > > > > accelerometer  |   1           |   1                  | cros-ec-accel.1
> > > > > gyroscope        |    0          |    2                 | cros-ec-gyro.0
> > > > > magnetometer |    0          |   3                  | cros-ec-mag.0
> > > > > light                  |    0          |   4                  | cros-ec-light.0
> > > > > ...
> > > >
> > > > Why can't these numbers be assigned at runtime?
> > > I assume you want to know why IIO drivers need to know "sensor_num"
> > > ahead of time. It is because each IIO driver is independent from the
> > > other.
> > > Let assume there was 2 light sensors in the device:
> > > type                  |   id          | sensor_num   | device name
> > >  light                  |    0          |   4                  | cros-ec-light.0
> > >  light                  |    1          |   5                  | cros-ec-light.1
> > >
> > > In case of sensors of the same type without sensor_num, cros-ec-light
> > > driver has no information at probe time if it should bind to sensors
> > > named by the EC 4 or 5.
> > >
> > > We could get away with cros-ec-accel, as EC always presents
> > > accelerometers with sensor_num  0 and 1, but I don't want to rely on
> > > this property in the general case.
> > > Only cros_ec_dev MFD driver has the global view of all sensors available.
> >
> > Well seeing as this implementation has already been accepted and you're
> > only *using* it, rather than creating it, I think this conversation is
> > moot.  It looks like the original implementation patch was not
> > reviewed by me, which is frustrating since I would have NACKed it.
> >
> > Just so you know, pointlessly enumerating identical devices manually
> > is not a good practice.  It is one we reject all the time.  This
> > imp. should too have been rejected on submission.

> I wrote the original code, Enric submitted it, so I am not just using it.

My point was, *this* patch is just using it.  The implementation has
already been applied to the mainline kernel.  Who wrote the initial
commit is not important at this point.

> We can work on implementing the right way. Which model should I follow?
> The code function is similar to HID sensor hub code which is done in
> driver/hid/hid-sensor-hub.c [sensor_hub_probe()] which calls
> mfd_add_hotplug_devices() with an array of mfd_cell,
> hid_sensor_hub_client_devs. Each cell platfom_data contains  a hsdev
> structure that is shared between the iio driver and the hid sensor hub
> driver. hsdev->usage information is sent back and forth between
> specialized hid IIO device driver and the HID sensor hub driver, for
> example when sensor_hub_input_attr_get_raw_value() is called.
> hsdev->usage has the same usage a sensor_num I am using.

It looks like the HID Usage implementation is using a set of
pre-defined values to identify sensor *types*:

  include/linux/hid-sensor-ids.h

Where as your implementation is confusing me.  In some instances you
are using it as what looks like an *index* into a register set:

  ec_cmd_read_u16(st->ec,
                EC_MEMMAP_ACC_DATA +
                sizeof(s16) *
                (1 + i + st->sensor_num * MAX_AXIS),
                data);

And at other times it is used for sensor *types*, but in a very
limited way:

  enum motionsensor_location {
          MOTIONSENSE_LOC_BASE = 0,
          MOTIONSENSE_LOC_LID = 1,
          MOTIONSENSE_LOC_MAX,
  };

  static char *cros_ec_accel_legacy_loc_strings[] = {
          [MOTIONSENSE_LOC_BASE] = "base",
          [MOTIONSENSE_LOC_LID] = "lid",
          [MOTIONSENSE_LOC_MAX] = "unknown",
  };

  return sprintf(buf, "%s\n",
                 cros_ec_accel_legacy_loc_strings[st->sensor_num +
                                                MOTIONSENSE_LOC_BASE]);

> I am not enumerating identical devices twice: the embedded controller
> manages a list of sensors:
> 
> For instance on pixelbook, it look like:
>        +--------+
>         | EC    |
>        +--------+
>    ( via several i2c/spi buses)
> +--------------------+--------------+-------- ...
> |                         |                  |
> IMU (base)     light/prox    Accelrometer (lid)
> |
> Magnetometer
> 
> A given hardware sensor may be composed of multiple logical sensors
> (IMU is a accelerometer and a gyroscope into one package).
> 
> The EC firmware list all the (logical) sensors in array, and that
> unique index - sensor_num - points to a single logical sensor.

What what is 'sensor_num'; is it a channel address/number similar to
what I2C HIDs use to communicate over a specific I2C line, or is it a
type, similar to what HID devices provide on request for
identification purposes? 

> Is it more acceptable if I use PLATFORM_DEVID_AUTO instead of
> assigning .id myself?

Is this a separate question, or can 'sensor_num' be any unique
arbitrary number?

> The topology will look like:
> find . -type d -name \*auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-logger.8.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-accel.2.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.4.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-usbpd-charger.7.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-gyro.3.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-mag.5.auto
> ./devices/pci0000:00/0000:00:1f.0/PNP0C09:00/GOOG0004:00/cros-ec-dev.1.auto/cros-ec-ring.6.auto
> ./devices/pci0000:00/0000:00:15.2/i2c_designware.2/i2c-8/i2c-GOOG0008:00/cros-ec-dev.0.auto
> 
> Thank you for your support,

No problem.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog