Re: [PATCH v4 5/8] mfd: cros_ec_dev: Register cros_ec_accel_legacy driver as a subdevice.

[Gwendal Grignou]

On Mon, Apr 16, 2018 at 6:21 AM Lee Jones <lee.jones@xxxxxxxxxx> wrote:
>
> On Wed, 04 Apr 2018, Enric Balletbo Serra wrote:
>
> > Hi again,
> >
> > 2018-04-04 10:03 GMT+02:00 Enric Balletbo Serra <eballetbo@xxxxxxxxx>:
> > > Hi Lee,
> > >
> > > 2018-03-28 13:03 GMT+02:00 Lee Jones <lee.jones@xxxxxxxxxx>:
> > >> On Tue, 20 Mar 2018, Enric Balletbo i Serra wrote:
> > >>
> > >>> 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@xxxxxxxxxxxx>
> > >>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
> > >>> Reviewed-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
> > >>> Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > >>> ---
> > >>>
> > >>> 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 | 70 +++++++++++++++++++++++++++++++++++++++++++++++
> > >>>  1 file changed, 70 insertions(+)
> > >>>
> > >>> diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
> > >>> index f60a53f11942..0d541d59d6f5 100644
> > >>> --- a/drivers/mfd/cros_ec_dev.c
> > >>> +++ b/drivers/mfd/cros_ec_dev.c
> > >>> @@ -389,6 +389,73 @@ 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,
> > >>> +     }
> > >>> +};
> > >>
> > >> Also no need to be so many  lines.
> > >>
> > >> Each one of these entries can be placed on a single line.
> > >>
> > >> And there's no need for a comma if there is nothing to separate.
> > >>
> > >>         { .sensor_num = 0 },
> > >>         { .sensor_num = 1 }
> > >>
> > >> Also, this seems like a pretty pointless struct.
> > >>
> > >> What is the sensor_num property used for?
> > >>
> > >> Why does it care what sensor number it is?
This patch add support for device with older embedded controller
firmware that do not support the command MOTIONSENSE_CMD_DUMP.
Contrary to cros_ec_sensors_register(), we need to guess what
MOTIONSENSE_CMD_DUMP would have returned when only 2 accelerometers
are present.

The IIO CrosEC sensors driver needs these id created by the EC to
reference a particular sensor: this is sensor_num in
cros_ec_sensor_platform.
It happens to be the same as .id in mfd_cell.
> > >>
> > >
> > > I thought that was used but after look again I didn't see where, so
> > > seems that you have reason and this struct is pointless. I'll remove
> > > this and the .id in the cells, we can always send a patch later
> > > introducing this if I am missing something. I'll send another version.
> > >
> >
> > Ok, forget what I said.
> >
> > Actually, sensor_num is used by the cros_ec_legacy driver to get the
> > right sensor properties from the EC and to get the sensor data from
> > the EC, sensor_num is the offset passed to the EC read command.
>
> I'm sure that it is being used, but my question is, why?
>
> Passing a 0 and a 1 to a child driver seems like a pointless exercise
> to me.  We do not usually enumerate devices like this with platform
> data.  Do these values ever change, or are they simply used to
> enumerate 2 devices which are always called 0 and 1?
The later. The IIO driver needs to use the EC assigned names 0 and 1.
>
> > .id is not used but as there are two accelerometers that use the same
> > driver, shouldn't we set the id (or I am missing something)?
It is just a coincidence that .id and .sensor_num are identical.
> >
> > +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> > +     { .name = "cros-ec-accel-legacy", .id = 0 },
> > +     { .name = "cros-ec-accel-legacy", .id = 1 },
> > + };
I repost a more concise patch.

Thanks,

Gwendal.
>
> I think you need to understand what MFD/Platform code does with these
> IDs.  Take a look at the MFD/Platform core code to enlighten
> yourself.  If there's anything you then do not understand, please
> ask and I'll try to fill in the gaps.
>
> > >>> +static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
> > >>> +     {
> > >>> +             .name = "cros-ec-accel-legacy",
> > >>> +             .id = 0,
> > >>
> > >> What are you using this for?
> > >>
> > >>> +             .platform_data = &sensor_platforms[0],
> > >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> > >>> +     },
> > >>> +     {
> > >>> +             .name = "cros-ec-accel-legacy",
> > >>> +             .id = 1,
> > >>
> > >> And this?
> > >>
> > >>> +             .platform_data = &sensor_platforms[1],
> > >>> +             .pdata_size = sizeof(struct cros_ec_sensor_platform),
> > >>> +     }
> > >>> +};
> > >>> +
> > >>> +static void cros_ec_accel_legacy_register(struct cros_ec_dev *ec)
> > >>> +{
> > >>> +     struct cros_ec_device *ec_dev = ec->ec_dev;
> > >>> +     u8 status;
> > >>> +     int ret;
> > >>> +
> > >>> +     /*
> > >>> +      * ECs that need legacy support are the main EC, directly connected to
> > >>> +      * the AP.
> > >>> +      */
> > >>> +     if (ec->cmd_offset != 0)
> > >>> +             return;
> > >>> +
> > >>> +     /*
> > >>> +      * Check if EC supports direct memory reads and if EC has
> > >>> +      * accelerometers.
> > >>> +      */
> > >>> +     if (!ec_dev->cmd_readmem)
> > >>> +             return;
> > >>> +
> > >>> +     ret = ec_dev->cmd_readmem(ec_dev, EC_MEMMAP_ACC_STATUS, 1, &status);
> > >>> +     if (ret < 0) {
> > >>> +             dev_warn(ec->dev, "EC does not support direct reads.\n");
> > >>> +             return;
> > >>> +     }
> > >>> +
> > >>> +     /* Check if EC has accelerometers. */
> > >>> +     if (!(status & EC_MEMMAP_ACC_STATUS_PRESENCE_BIT)) {
> > >>> +             dev_info(ec->dev, "EC does not have accelerometers.\n");
> > >>> +             return;
> > >>> +     }
> > >>> +
> > >>> +     /*
> > >>> +      * Register 2 accelerometers
> > >>> +      */
> > >>> +     ret = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> > >>> +                           cros_ec_accel_legacy_cells,
> > >>> +                           ARRAY_SIZE(cros_ec_accel_legacy_cells),
> > >>> +                           NULL, 0, NULL);
> > >>> +     if (ret)
> > >>> +             dev_err(ec_dev->dev, "failed to add EC sensors\n");
> > >>> +}
> > >>> +
> > >>>  static const struct mfd_cell cros_ec_rtc_cells[] = {
> > >>>       {
> > >>>               .name = "cros-ec-rtc",
> > >>> @@ -442,6 +509,9 @@ static int ec_device_probe(struct platform_device *pdev)
> > >>>       /* check whether this EC is a sensor hub. */
> > >>>       if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
> > >>>               cros_ec_sensors_register(ec);
> > >>> +     else
> > >>> +             /* Workaroud for older EC firmware */
> > >>> +             cros_ec_accel_legacy_register(ec);
> > >>>
> > >>>       /* Check whether this EC instance has RTC host command support */
> > >>>       if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
> > >>
>
> --
> Lee Jones [æçæ]
> Linaro Services Technical Lead
> Linaro.org â Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog