Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

[Dmitry Torokhov]

On Tue, Jan 03, 2017 at 07:50:17PM +0100, Pali RohÃr wrote:
> On Tuesday 03 January 2017 19:38:43 Dmitry Torokhov wrote:
> > On Tue, Jan 03, 2017 at 10:06:41AM +0100, Benjamin Tissoires wrote:
> > > On Dec 29 2016 or thereabouts, Pali RohÃr wrote:
> > > > On Thursday 29 December 2016 22:09:32 MichaÅ KÄpieÅ wrote:
> > > > > > On Thursday 29 December 2016 14:47:19 MichaÅ KÄpieÅ wrote:
> > > > > > > > On Thursday 29 December 2016 09:29:36 MichaÅ KÄpieÅ wrote:
> > > > > > > > > > Dell platform team told us that some (DMI
> > > > > > > > > > whitelisted) Dell Latitude machines have ST
> > > > > > > > > > microelectronics accelerometer at i2c address 0x29.
> > > > > > > > > > That i2c address is not specified in DMI or ACPI, so
> > > > > > > > > > runtime detection without whitelist which is below
> > > > > > > > > > is not possible.
> > > > > > > > > > 
> > > > > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > > > > represent that accelerometer. Unfortunately without
> > > > > > > > > > i2c address.
> > > > > > > > > 
> > > > > > > > > This part of the commit message sounded a bit confusing
> > > > > > > > > to me at first because there is already an ACPI driver
> > > > > > > > > which handles SMO88xx
> > > > > > > > > 
> > > > > > > > > devices (dell-smo8800).  My understanding is that:
> > > > > > > > >   * the purpose of this patch is to expose a richer
> > > > > > > > >   interface (as
> > > > > > > > >   
> > > > > > > > >     provided by lis3lv02d) to these devices on some
> > > > > > > > >     machines,
> > > > > > > > >   
> > > > > > > > >   * on whitelisted machines, dell-smo8800 and lis3lv02d
> > > > > > > > >   can work
> > > > > > > > >   
> > > > > > > > >     simultaneously (even though dell-smo8800
> > > > > > > > >     effectively duplicates the work that lis3lv02d
> > > > > > > > >     does).
> > > > > > > > 
> > > > > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > > > > /dev/freefall device which notify userspace about falls.
> > > > > > > > lis3lv02d is i2c driver which exports axes of
> > > > > > > > accelerometer. Additionaly lis3lv02d can export also
> > > > > > > > /dev/freefall if registerer of i2c device provides irq
> > > > > > > > number -- which is not case of this patch.
> > > > > > > > 
> > > > > > > > So both drivers are doing different things and both are
> > > > > > > > useful.
> > > > > > > > 
> > > > > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW
> > > > > > > > device (that ST microelectronics accelerometer) but due
> > > > > > > > to complicated HW abstraction and layers on Dell laptops
> > > > > > > > it is handled by two drivers, one ACPI and one i2c.
> > > > > > > > 
> > > > > > > > Yes, in ideal world irq number should be passed to
> > > > > > > > lis3lv02d driver and that would export whole device
> > > > > > > > (with /dev/freefall too), but due to HW abstraction it
> > > > > > > > is too much complicated...
> > > > > > > 
> > > > > > > Why?  AFAICT, all that is required to pass that IRQ number
> > > > > > > all the way down to lis3lv02d is to set the irq field of
> > > > > > > the struct i2c_board_info you are passing to
> > > > > > > i2c_new_device().  And you can extract that IRQ number
> > > > > > > e.g. in check_acpi_smo88xx_device(). However, you would
> > > > > > > then need to make sure dell-smo8800 does not attempt to
> > > > > > > request the same IRQ on whitelisted machines.  This got me
> > > > > > > thinking about a way to somehow incorporate your changes
> > > > > > > into dell-smo8800 using Wolfram's bus_notifier suggestion,
> > > > > > > but I do not have a working solution for now.  What is
> > > > > > > tempting about this approach is that you would not have to
> > > > > > > scan the ACPI namespace in search of SMO88xx devices,
> > > > > > > because smo8800_add() is automatically called for them. 
> > > > > > > However, I fear that the resulting solution may be more
> > > > > > > complicated than the one you submitted.
> > > > > > 
> > > > > > Then we need to deal with lot of problems. Order of loading
> > > > > > .ko modules is undefined. Binding devices to drivers
> > > > > > registered by .ko module is also in "random" order. At any
> > > > > > time any of those .ko module can be unloaded or at least
> > > > > > device unbind (via sysfs) from driver... And there can be
> > > > > > some pathological situation (thanks to adding ACPI layer as
> > > > > > Andy pointed) that there will be more SMO88xx devices in
> > > > > > ACPI. Plus you can compile kernel with and without those
> > > > > > modules and also you can blacklist loading them (so compile
> > > > > > time check is not enough). And still some correct message
> > > > > > notifier must be used.
> > > > > > 
> > > > > > I think such solution is much much more complicated, there
> > > > > > are lot of combinations of kernel configuration and
> > > > > > available dell devices...
> > > > > 
> > > > > I tried a few more things, but ultimately failed to find a nice
> > > > > way to implement this.
> > > > > 
> > > > > Another issue popped up, though.  Linus' master branch contains
> > > > > a recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a
> > > > > ("i2c: use an IRQ to report Host Notify events, not alert")
> > > > > which breaks your patch.  The reason for that is that
> > > > > lis3lv02d relies on the i2c client's IRQ being 0 to detect
> > > > > that it should not create /dev/freefall.  Benjamin's patch
> > > > > causes the Host Notify IRQ to be assigned to the i2c client
> > > > > your patch creates, thus causing lis3lv02d to create
> > > > > /dev/freefall, which in turn conflicts with dell-smo8800 which
> > > > > is trying to create /dev/freefall itself.
> > > > 
> > > > So 4d5538f5882a is breaking lis3lv02d driver...
> > > 
> > > Apologies for that.
> > > 
> > > I could easily fix this by adding a kernel API to know whether the
> > > provided irq is from Host Notify or if it was coming from an actual
> > > declaration. However, I have no idea how many other drivers would
> > > require this (hopefully only this one).
> > > 
> > > One other solution would be to reserve the Host Notify IRQ and let
> > > the actual drivers that need it to set it, but this was not the
> > > best solution according to Dmitri. On my side, I am not entirely
> > > against this given that it's a chip feature, so the driver should
> > > be able to know that it's available.
> > > 
> > > Dmitri, Wolfram, Jean, any preferences?
> > 
> > I read this:
> > 
> > "IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > ST microelectronics accelerometer) but due to complicated HW
> > abstraction and layers on Dell laptops it is handled by two drivers,
> > one ACPI and one i2c."
> > 
> > and that is the core of the issue. You have 2 drivers fighting over
> > the same device. Fix this and it will all work.
> 
> With my current implementation (which I sent in this patch), they are 
> not fighting.
> 
> dell-smo8800 exports /dev/freefall (and nothing more) and lis3lv02d only 
> accelerometer device as lis3lv02d driver does not get IRQ number in 
> platform data.
> 
> > As far as I can see hp_accel instantiates lis3lv02d and accesses it
> > via ACPI methods, can the same be done for Dell?
> 
> No, Dell does not have any ACPI methods. And as I wrote in ACPI or DMI 
> is even not i2c address of device, so it needs to be specified in code 
> itself.
> 
> Really there is no other way... :-(

Sure there is:

1. dell-smo8800 instantiates I2C device as "dell-smo8800-accel".
2. dell-smo8800 provides read/write functions for lis3lv02d that simply
   forward requests to dell-smo8800-accel i2c client.
3. dell-smo8800 instantiates lis3lv02d instance like hp_accel does.

Alternatively, can lis3lv02d be tasked to create /dev/freefall?

Yet another option: can we add a new flag to i2c_board_info controlling
whether we want to enable/disable wiring up host notify interrupt?
Benjamin, is there anything "special" in RMI SMbus ACPI descriptors we
could use?

Thanks.

-- 
Dmitry