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

[Enric Balletbo Serra]

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?
>>
>
> 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.

.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)?

+static const struct mfd_cell cros_ec_accel_legacy_cells[] = {
+     { .name = "cros-ec-accel-legacy", .id = 0 },
+     { .name = "cros-ec-accel-legacy", .id = 1 },
+ };

???

Thanks,
  Enric

> Thanks,
>   Enric
>
>>> +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