mbox series

[0/6] i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

Message ID 20231224213629.395741-1-hdegoede@redhat.com
Headers show
Series i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand

Message

Hans de Goede Dec. 24, 2023, 9:36 p.m. UTC
Hi All,

Here is a patch series which implements my suggestions from:
https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
to improve the lis3lv02d accel support on Dell laptops.

Jean, Andi the actual move is in patch 3/6 after some small prep patches
on the dell-smo8800 side. My plan for merging this is to create
an immutable branch based on 6.8-rc1 (once it is out) + these 6 patches and
then send a pull-request for this. Can I have your Ack for the i2c-i801
changes in patch 3/6? I think you'll like the changes there since they only
remove code :)

Regards,

Hans


Hans de Goede (6):
  platform/x86: dell-smo8800: Only load on Dell laptops
  platform/x86: dell-smo8800: Change probe() ordering a bit
  platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client
    from i2c-i801 to dell-smo8800
  platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client
  platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO
    st_accel driver
  platform/x86: dell-smo8800: Add support for probing for the
    accelerometer i2c address

 drivers/i2c/busses/i2c-i801.c            | 122 --------
 drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
 2 files changed, 316 insertions(+), 143 deletions(-)

Comments

Pali Rohár Dec. 24, 2023, 9:48 p.m. UTC | #1
On Sunday 24 December 2023 22:36:17 Hans de Goede wrote:
> The SMO8xxx ACPI HIDs are generic accelerometer ids which are also used
> by other vendors. Add a sys_vendor check to ensure that the dell-smo8800
> driver only loads on Dell laptops.

I saw that this driver was used also on some Acer laptops. As you wrote
above, and now after years I have also feeling that these ACPI HIDs are
generic, not Dell specific.

So I would propose to not restrict smo8800 driver just for Dell laptops
like this patch is introducing.

Maybe better option would be to move this driver out of the dell
namespace.

Are we able to collect dmesg outputs and verify this information on how
many non-dell laptops is this driver loaded?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index f7ec17c56833..4d5f778fb599 100644
> --- a/drivers/platform/x86/dell/dell-smo8800.c
> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> @@ -10,6 +10,7 @@
>  
>  #define DRIVER_NAME "smo8800"
>  
> +#include <linux/dmi.h>
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -108,6 +109,9 @@ static int smo8800_probe(struct platform_device *device)
>  	int err;
>  	struct smo8800_device *smo8800;
>  
> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> +		return false;
> +
>  	smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
>  	if (!smo8800) {
>  		dev_err(&device->dev, "failed to allocate device data\n");
> -- 
> 2.43.0
>
Pali Rohár Dec. 24, 2023, 10:03 p.m. UTC | #2
On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
> Instead of instantiating an i2c_client for the old misc joystick emulation
> and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
> i2c_client_id-s from the IIO st_accel driver so that the accelerometer
> gets presented to userspace as an IIO device like all modern accelerometer
> drivers do.
> 
> Add a new use_misc_lis3lv02d module-parameter which can be set to restore
> the old behavior in case someone has a use-case depending on this.
> 
> When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
> and disable the /dev/freefall chardev.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 82 ++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 4 deletions(-)

Sorry for the stupid question there, but what is the replacement for the
/dev/freefall when using new st_accel IIO driver?
Andy Shevchenko Dec. 25, 2023, 8 p.m. UTC | #3
On Sun, Dec 24, 2023 at 11:36 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO8xxx ACPI devices without an ACPI I2cResource
> inside the generic i801 I2C adapter driver.
>
> The kernel already instantiates platform_device-s for these ACPI devices
> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> platform drivers.
>
> Move the i2c_client instantiation from the generic i2c-i801 driver to
> the Dell specific dell-smo8800 driver.

...

> +       static const u16 i801_idf_pci_device_ids[] = {
> +               0x1d70, /* Patsburg (PCH) */
> +               0x1d71, /* Patsburg (PCH) */
> +               0x1d72, /* Patsburg (PCH) */
> +               0x8d7d, /* Wellsburg (PCH) */
> +               0x8d7e, /* Wellsburg (PCH) */
> +               0x8d7f, /* Wellsburg (PCH) */
> +       };

I prefer to see this as a PCI ID table (yes, I know the downsides).

...

> +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */
> +MODULE_SOFTDEP("pre: i2c-i801");

JFYI: software module dependencies are not supported by all kmod
implementations in the user space. I don't expect people to complain,
but just let you know that this kind of change needs to be done with
care.
Hans de Goede Jan. 5, 2024, 4:25 p.m. UTC | #4
Hi Pali,

Thank you for your feedback on this series.

On 12/24/23 22:48, Pali Rohár wrote:
> On Sunday 24 December 2023 22:36:17 Hans de Goede wrote:
>> The SMO8xxx ACPI HIDs are generic accelerometer ids which are also used
>> by other vendors. Add a sys_vendor check to ensure that the dell-smo8800
>> driver only loads on Dell laptops.
> 
> I saw that this driver was used also on some Acer laptops. As you wrote
> above, and now after years I have also feeling that these ACPI HIDs are
> generic, not Dell specific.
> 
> So I would propose to not restrict smo8800 driver just for Dell laptops
> like this patch is introducing.

Ok, I was not aware that some Acer laptops were also using this.

I was just trying to honor the vendor check done in
the i2c-i801 driver after the move of the i2c-client instantiation
to dell-smo8800.

Given that we are keeping the existing DMI matching I don't 
think keeping the sys-vendor check is necessary. So I'll
drop the patch from v2 of the series.

Regards,

Hans





> Maybe better option would be to move this driver out of the dell
> namespace.
> 
> Are we able to collect dmesg outputs and verify this information on how
> many non-dell laptops is this driver loaded?
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-smo8800.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>> index f7ec17c56833..4d5f778fb599 100644
>> --- a/drivers/platform/x86/dell/dell-smo8800.c
>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
>> @@ -10,6 +10,7 @@
>>  
>>  #define DRIVER_NAME "smo8800"
>>  
>> +#include <linux/dmi.h>
>>  #include <linux/fs.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/kernel.h>
>> @@ -108,6 +109,9 @@ static int smo8800_probe(struct platform_device *device)
>>  	int err;
>>  	struct smo8800_device *smo8800;
>>  
>> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>> +		return false;
>> +
>>  	smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
>>  	if (!smo8800) {
>>  		dev_err(&device->dev, "failed to allocate device data\n");
>> -- 
>> 2.43.0
>>
>
Hans de Goede Jan. 5, 2024, 4:34 p.m. UTC | #5
Hi,

On 12/24/23 23:03, Pali Rohár wrote:
> On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
>> Instead of instantiating an i2c_client for the old misc joystick emulation
>> and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
>> i2c_client_id-s from the IIO st_accel driver so that the accelerometer
>> gets presented to userspace as an IIO device like all modern accelerometer
>> drivers do.
>>
>> Add a new use_misc_lis3lv02d module-parameter which can be set to restore
>> the old behavior in case someone has a use-case depending on this.
>>
>> When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
>> and disable the /dev/freefall chardev.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-smo8800.c | 82 ++++++++++++++++++++++--
>>  1 file changed, 78 insertions(+), 4 deletions(-)
> 
> Sorry for the stupid question there, but what is the replacement for the
> /dev/freefall when using new st_accel IIO driver?

There is no replacement for /dev/freefall. I realize this is not ideal
and if this turns out to be a problem the default of the module option
can be reverted.

But AFAIK / AFAICT there are no actual userspace consumers of
/dev/freefall so removing it should not be an issue.

Specifically I checked smartmontools which ships smartd which
is the only daemon which I know of for hdd monitoring and
that does not have /dev/freefall support. So /dev/freefall
appears to be unused to me ?

For completeness I also checked libatasmart which also does
not access /dev/freefall.

Regards,

Hans
Pali Rohár Jan. 5, 2024, 6:33 p.m. UTC | #6
On Friday 05 January 2024 17:34:07 Hans de Goede wrote:
> Hi,
> 
> On 12/24/23 23:03, Pali Rohár wrote:
> > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
> >> Instead of instantiating an i2c_client for the old misc joystick emulation
> >> and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
> >> i2c_client_id-s from the IIO st_accel driver so that the accelerometer
> >> gets presented to userspace as an IIO device like all modern accelerometer
> >> drivers do.
> >>
> >> Add a new use_misc_lis3lv02d module-parameter which can be set to restore
> >> the old behavior in case someone has a use-case depending on this.
> >>
> >> When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
> >> and disable the /dev/freefall chardev.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/platform/x86/dell/dell-smo8800.c | 82 ++++++++++++++++++++++--
> >>  1 file changed, 78 insertions(+), 4 deletions(-)
> > 
> > Sorry for the stupid question there, but what is the replacement for the
> > /dev/freefall when using new st_accel IIO driver?
> 
> There is no replacement for /dev/freefall.

That is a big problem if there is no replacement. The primary usage of
that hardware and all these drivers is that freefall ability. It was
originally written for HP laptops and later extended for Dell. Access to
accelerometer axes was just a secondary functions for linux fans.

> I realize this is not ideal
> and if this turns out to be a problem the default of the module option
> can be reverted.
> 
> But AFAIK / AFAICT there are no actual userspace consumers of
> /dev/freefall so removing it should not be an issue.

Userspace tool is directly in the kernel tree. Somewhere in tools dir
now as it was moved (if it was not removed).

> Specifically I checked smartmontools which ships smartd which
> is the only daemon which I know of for hdd monitoring and
> that does not have /dev/freefall support. So /dev/freefall
> appears to be unused to me ?
> 
> For completeness I also checked libatasmart which also does
> not access /dev/freefall.

I guess nobody ported it to these tools. IIRC the freefall design comes
from the Suse the tool was also used on preloaded HP laptops. So maybe
Suse have custom tool? I do not know.

> Regards,
> 
> Hans
> 
>
Andy Shevchenko Jan. 5, 2024, 6:37 p.m. UTC | #7
On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 12/24/23 23:03, Pali Rohár wrote:
> > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:

...

> But AFAIK / AFAICT there are no actual userspace consumers of
> /dev/freefall so removing it should not be an issue.

IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.
Andy Shevchenko Jan. 5, 2024, 7:04 p.m. UTC | #8
On Fri, Jan 5, 2024 at 8:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 12/24/23 23:03, Pali Rohár wrote:
> > > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:

...

> > But AFAIK / AFAICT there are no actual userspace consumers of
> > /dev/freefall so removing it should not be an issue.
>
> IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.

Okay, I can't google for it and now I realised that it was my x60s,
which has no freefall, but another interface to it. In any case the
side effect of that googling is this (maybe more, I just took this one
as example):
https://github.com/linux-thinkpad/hdapsd/blob/master/README.md

So, dropping it will break at least this tool.
Pali Rohár Jan. 5, 2024, 7:20 p.m. UTC | #9
On Friday 05 January 2024 21:04:59 Andy Shevchenko wrote:
> On Fri, Jan 5, 2024 at 8:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > > On 12/24/23 23:03, Pali Rohár wrote:
> > > > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
> 
> ...
> 
> > > But AFAIK / AFAICT there are no actual userspace consumers of
> > > /dev/freefall so removing it should not be an issue.
> >
> > IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.
> 
> Okay, I can't google for it and now I realised that it was my x60s,
> which has no freefall, but another interface to it. In any case the
> side effect of that googling is this (maybe more, I just took this one
> as example):
> https://github.com/linux-thinkpad/hdapsd/blob/master/README.md
> 
> So, dropping it will break at least this tool.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Yes, this is that correct one. I forget the name of this daemon.

Just to note /dev/freefall does not provide axes state, it just send
signal to process when interrupt is triggered. Process than park disk
heads.

Axes state are/were exported throw /dev/js* interface and those games
uses just js interface. I remember Tux Racer.

Interrupt on HP and Dell is triggered only when laptop fall is detected,
so games did not used it (hopefully!)
Hans de Goede Jan. 5, 2024, 7:46 p.m. UTC | #10
Hi,

On 1/5/24 20:20, Pali Rohár wrote:
> On Friday 05 January 2024 21:04:59 Andy Shevchenko wrote:
>> On Fri, Jan 5, 2024 at 8:37 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 12/24/23 23:03, Pali Rohár wrote:
>>>>> On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
>>
>> ...
>>
>>>> But AFAIK / AFAICT there are no actual userspace consumers of
>>>> /dev/freefall so removing it should not be an issue.
>>>
>>> IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.
>>
>> Okay, I can't google for it and now I realised that it was my x60s,
>> which has no freefall, but another interface to it. In any case the
>> side effect of that googling is this (maybe more, I just took this one
>> as example):
>> https://github.com/linux-thinkpad/hdapsd/blob/master/README.md
>>
>> So, dropping it will break at least this tool.
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
> 
> Yes, this is that correct one. I forget the name of this daemon.
> 
> Just to note /dev/freefall does not provide axes state, it just send
> signal to process when interrupt is triggered. Process than park disk
> heads.
> 
> Axes state are/were exported throw /dev/js* interface and those games
> uses just js interface. I remember Tux Racer.
> 
> Interrupt on HP and Dell is triggered only when laptop fall is detected,
> so games did not used it (hopefully!)

Ok, so I clearly need to change the module parameter so that we stick with
the drivers/char/misc/lis3lv02d driver as default and offer using
the i2c-client-id which results in loading the iio driver instead as
an option enabled by a module parameter.

I'll fix this for v2.

Regards,

Hans
Paul Menzel Jan. 6, 2024, 2:23 p.m. UTC | #11
Dear Hans,


Thank you very much for working on this issue and even sending patches 
so quickly.


Am 24.12.23 um 22:36 schrieb Hans de Goede:

> Here is a patch series which implements my suggestions from:
> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
> to improve the lis3lv02d accel support on Dell laptops.
> 
> Jean, Andi the actual move is in patch 3/6 after some small prep patches
> on the dell-smo8800 side. My plan for merging this is to create
> an immutable branch based on 6.8-rc1 (once it is out) + these 6 patches and
> then send a pull-request for this. Can I have your Ack for the i2c-i801
> changes in patch 3/6? I think you'll like the changes there since they only
> remove code :)

> Hans de Goede (6):
>    platform/x86: dell-smo8800: Only load on Dell laptops
>    platform/x86: dell-smo8800: Change probe() ordering a bit
>    platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
>    platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client
>    platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver
>    platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address
> 
>   drivers/i2c/busses/i2c-i801.c            | 122 --------
>   drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
>   2 files changed, 316 insertions(+), 143 deletions(-)

This Thursday, I tested this on the Dell Inc. XPS 15 7590/0VYV0G, BIOS 
1.24.0 09/11/2023.

First just with your patch-set and trying the parameter:

     [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-6.7.0-rc8+ 
root=UUID=9fa41e21-7a5f-479e-afdc-9a5503368d8e ro quiet rd.luks=1 
rd.auto=1 dell-smo8800.probe_i2c_addr=0x29
     […]
     [   28.826356] smo8800 SMO8810:00: Accelerometer lis3lv02d is 
present on SMBus but its address is unknown, skipping registration
     [   28.826359] smo8800 SMO8810:00: Pass 
dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this 
may be dangerous!

I misread the parameter documentation, but didn’t see the message back 
then, and just saved the log files.

So, I added an entry for the device, and got:

     [   19.197838] smo8800 SMO8810:00: Registered lis2de12 
accelerometer on address 0x29


Kind regards,

Paul


PS: I still seem to miss some config option in my custom Linux kernel 
configuration, as with my self-built Linux kernel, the accelerometer is 
not detected as an input device.

```
$ sudo dmesg | grep input
[    0.648449] input: AT Translated Set 2 keyboard as 
/devices/platform/i8042/serio0/input/input0
[   19.164633] input: Intel HID events as 
/devices/platform/INT33D5:00/input/input2
[   19.176109] input: Intel HID 5 button array as 
/devices/platform/INT33D5:00/input/input3
[   19.200645] input: Lid Switch as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0D:00/input/input4
[   19.230941] input: Power Button as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input5
[   19.231434] input: Sleep Button as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0E:00/input/input6
[   19.350390] input: PC Speaker as /devices/platform/pcspkr/input/input7
[   19.996196] input: Dell WMI hotkeys as 
/devices/platform/PNP0C14:05/wmi_bus/wmi_bus-PNP0C14:05/9DBB5994-A997-11DA-B012-B622A1EF5492/input/input9
[   20.014546] input: SYNA2393:00 06CB:7A13 Mouse as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input10
[   20.047534] input: SYNA2393:00 06CB:7A13 Touchpad as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input11
[   20.047667] hid-generic 0018:06CB:7A13.0001: input,hidraw0: I2C HID 
v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
[   20.048874] input: WCOM490B:00 056A:490B Touchscreen as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input13
[   20.049014] input: WCOM490B:00 056A:490B as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input14
[   20.049089] input: WCOM490B:00 056A:490B Stylus as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input15
[   20.049186] input: WCOM490B:00 056A:490B as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input16
[   20.065066] input: WCOM490B:00 056A:490B Mouse as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input17
[   20.065360] hid-generic 0018:056A:490B.0002: input,hidraw1: I2C HID 
v1.00 Mouse [WCOM490B:00 056A:490B] on i2c-WCOM490B:00
[   20.760879] input: SYNA2393:00 06CB:7A13 Mouse as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input18
[   20.760979] input: SYNA2393:00 06CB:7A13 Touchpad as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input19
[   20.761032] hid-multitouch 0018:06CB:7A13.0001: input,hidraw0: I2C 
HID v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
[   21.083016] input: Wacom HID 490B Pen as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input21
[   21.083149] input: Wacom HID 490B Finger as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input22
[   25.850198] input: Video Bus as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input24
[   25.850344] input: Video Bus as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:0b/LNXVIDEO:01/input/input25
[   26.027649] snd_hda_codec_realtek hdaudioC0D0:    inputs:
[   26.132076] input: HDA Intel PCH Headphone Mic as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input26
[   26.132148] input: HDA Intel PCH HDMI/DP,pcm=3 as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input27
[   26.132192] input: HDA Intel PCH HDMI/DP,pcm=7 as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input28
[   26.132233] input: HDA Intel PCH HDMI/DP,pcm=8 as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input29
[   28.659169] rfkill: input handler disabled
[   47.283492] rfkill: input handler enabled
[   52.883611] rfkill: input handler disabled
```
Hans de Goede Jan. 6, 2024, 4:15 p.m. UTC | #12
Hi Paul,

On 1/6/24 15:23, Paul Menzel wrote:
> Dear Hans,
> 
> 
> Thank you very much for working on this issue and even sending patches so quickly.

You're welcome this was a nice little project to work on during the holidays.

> Am 24.12.23 um 22:36 schrieb Hans de Goede:
> 
>> Here is a patch series which implements my suggestions from:
>> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
>> to improve the lis3lv02d accel support on Dell laptops.
>>
>> Jean, Andi the actual move is in patch 3/6 after some small prep patches
>> on the dell-smo8800 side. My plan for merging this is to create
>> an immutable branch based on 6.8-rc1 (once it is out) + these 6 patches and
>> then send a pull-request for this. Can I have your Ack for the i2c-i801
>> changes in patch 3/6? I think you'll like the changes there since they only
>> remove code :)
> 
>> Hans de Goede (6):
>>    platform/x86: dell-smo8800: Only load on Dell laptops
>>    platform/x86: dell-smo8800: Change probe() ordering a bit
>>    platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
>>    platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client
>>    platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver
>>    platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address
>>
>>   drivers/i2c/busses/i2c-i801.c            | 122 --------
>>   drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
>>   2 files changed, 316 insertions(+), 143 deletions(-)
> 
> This Thursday, I tested this on the Dell Inc. XPS 15 7590/0VYV0G, BIOS 1.24.0 09/11/2023.

Interesting, can you run:

sudo acpidump -o acpidump.txt and then send me a private email
with the generated acpidump.txt attached ?

> 
> First just with your patch-set and trying the parameter:
> 
>     [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-6.7.0-rc8+ root=UUID=9fa41e21-7a5f-479e-afdc-9a5503368d8e ro quiet rd.luks=1 rd.auto=1 dell-smo8800.probe_i2c_addr=0x29
>     […]
>     [   28.826356] smo8800 SMO8810:00: Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration
>     [   28.826359] smo8800 SMO8810:00: Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!
> 
> I misread the parameter documentation, but didn’t see the message back then, and just saved the log files.
> 
> So, I added an entry for the device, and got:
> 
>     [   19.197838] smo8800 SMO8810:00: Registered lis2de12 accelerometer on address 0x29

Ok, that looks good. Can you provide the output of:

cat /sys/class/dmi/id/product_name

So that we can also add an entry for this upstream ?

> PS: I still seem to miss some config option in my custom Linux kernel configuration, as with my self-built Linux kernel, the accelerometer is not detected as an input device.

Right, v1 of my patches changed the code to by default instantiate an i2c_client
to which the st_accel IIO driver will bind. Using the IIO framework is
how accelerometers are handled normally and the handling of these "freefall" 
sensors so far has been a bit different, so I tried to make them more like
normal accelerometers which don't do the joystick emulation.

But Pali and Andy pointed out to me that there is userspace code out
there relying on /dev/freefall, so for v2 of the patches I've kept
the old behavior by default.

I've just posted v2 of the patches.

Note with v1 you can also get the old behavior by adding
dell_smo8800.use_misc_lis3lv02d=1 to the kernel commandline.

Adding that (or switching to the v2 patches) should give you
an input device.

Regards,

Hans






> 
> ```
> $ sudo dmesg | grep input
> [    0.648449] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
> [   19.164633] input: Intel HID events as /devices/platform/INT33D5:00/input/input2
> [   19.176109] input: Intel HID 5 button array as /devices/platform/INT33D5:00/input/input3
> [   19.200645] input: Lid Switch as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0D:00/input/input4
> [   19.230941] input: Power Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input5
> [   19.231434] input: Sleep Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0E:00/input/input6
> [   19.350390] input: PC Speaker as /devices/platform/pcspkr/input/input7
> [   19.996196] input: Dell WMI hotkeys as /devices/platform/PNP0C14:05/wmi_bus/wmi_bus-PNP0C14:05/9DBB5994-A997-11DA-B012-B622A1EF5492/input/input9
> [   20.014546] input: SYNA2393:00 06CB:7A13 Mouse as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input10
> [   20.047534] input: SYNA2393:00 06CB:7A13 Touchpad as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input11
> [   20.047667] hid-generic 0018:06CB:7A13.0001: input,hidraw0: I2C HID v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
> [   20.048874] input: WCOM490B:00 056A:490B Touchscreen as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input13
> [   20.049014] input: WCOM490B:00 056A:490B as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input14
> [   20.049089] input: WCOM490B:00 056A:490B Stylus as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input15
> [   20.049186] input: WCOM490B:00 056A:490B as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input16
> [   20.065066] input: WCOM490B:00 056A:490B Mouse as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input17
> [   20.065360] hid-generic 0018:056A:490B.0002: input,hidraw1: I2C HID v1.00 Mouse [WCOM490B:00 056A:490B] on i2c-WCOM490B:00
> [   20.760879] input: SYNA2393:00 06CB:7A13 Mouse as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input18
> [   20.760979] input: SYNA2393:00 06CB:7A13 Touchpad as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input19
> [   20.761032] hid-multitouch 0018:06CB:7A13.0001: input,hidraw0: I2C HID v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
> [   21.083016] input: Wacom HID 490B Pen as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input21
> [   21.083149] input: Wacom HID 490B Finger as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input22
> [   25.850198] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input24
> [   25.850344] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:0b/LNXVIDEO:01/input/input25
> [   26.027649] snd_hda_codec_realtek hdaudioC0D0:    inputs:
> [   26.132076] input: HDA Intel PCH Headphone Mic as /devices/pci0000:00/0000:00:1f.3/sound/card0/input26
> [   26.132148] input: HDA Intel PCH HDMI/DP,pcm=3 as /devices/pci0000:00/0000:00:1f.3/sound/card0/input27
> [   26.132192] input: HDA Intel PCH HDMI/DP,pcm=7 as /devices/pci0000:00/0000:00:1f.3/sound/card0/input28
> [   26.132233] input: HDA Intel PCH HDMI/DP,pcm=8 as /devices/pci0000:00/0000:00:1f.3/sound/card0/input29
> [   28.659169] rfkill: input handler disabled
> [   47.283492] rfkill: input handler enabled
> [   52.883611] rfkill: input handler disabled
> ```
>