linux-kernel - Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190529114454.GJ4574@dell>
Date:   Wed, 29 May 2019 12:44:54 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     Gwendal Grignou <gwendal@...omium.org>
Cc:     andy.shevchenko@...il.com, Guenter Roeck <groeck@...omium.org>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, kernel@...labora.com
Subject: Re: [PATCH v5] mfd: cros_ec_dev: Register cros_ec_accel_legacy
 driver as a subdevice

On Tue, 28 May 2019, Gwendal Grignou wrote:

> On Mon, Apr 1, 2019 at 8:46 PM Lee Jones <lee.jones@...aro.org> wrote:
> >
> > On Wed, 27 Feb 2019, Gwendal Grignou wrote:
> >
> > > From: Enric Balletbo i Serra <enric.balletbo@...labora.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@...omium.org>
> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.com>
> > > Reviewed-by: Gwendal Grignou <gwendal@...omium.org>
> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.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?

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