diff mbox series

[5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

Message ID 20231224213629.395741-6-hdegoede@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand

Commit Message

Hans de Goede Dec. 24, 2023, 9:36 p.m. UTC
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(-)

Comments

Pali Rohár Dec. 24, 2023, 10:03 p.m. UTC | #1
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?
Hans de Goede Jan. 5, 2024, 4:34 p.m. UTC | #2
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 | #3
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 | #4
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 | #5
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 | #6
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 | #7
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
kernel test robot Jan. 9, 2024, 2 a.m. UTC | #8
Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.7]
[cannot apply to wsa/i2c/for-next next-20240108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/platform-x86-dell-smo8800-Only-load-on-Dell-laptops/20231225-152720
base:   linus/master
patch link:    https://lore.kernel.org/r/20231224213629.395741-6-hdegoede%40redhat.com
patch subject: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver
config: i386-randconfig-003-20240106 (https://download.01.org/0day-ci/archive/20240109/202401090941.FHkrtPXf-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/202401090941.FHkrtPXf-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401090941.FHkrtPXf-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_remove':
   drivers/platform/x86/dell/dell-smo8800.c:358: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.c:358: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:243: undefined reference to `i2c_bus_type'
   ld: drivers/platform/x86/dell/dell-smo8800.c:286: undefined reference to `i2c_put_adapter'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_detect_accel':
>> drivers/platform/x86/dell/dell-smo8800.c:170: undefined reference to `i2c_smbus_xfer'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:276: undefined reference to `i2c_new_client_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_probe':
   drivers/platform/x86/dell/dell-smo8800.c:345: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_find_i801':
   drivers/platform/x86/dell/dell-smo8800.c:131: undefined reference to `i2c_verify_adapter'
   ld: drivers/platform/x86/dell/dell-smo8800.c:145: undefined reference to `i2c_get_adapter'


vim +170 drivers/platform/x86/dell/dell-smo8800.c

   161	
   162	static int smo8800_detect_accel(struct smo8800_device *smo8800,
   163					struct i2c_adapter *adap, u8 addr,
   164					struct i2c_board_info *info)
   165	{
   166		union i2c_smbus_data smbus_data;
   167		const char *type;
   168		int err;
   169	
 > 170		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
   171				     I2C_SMBUS_BYTE_DATA, &smbus_data);
   172		if (err < 0) {
   173			dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
   174			return err;
   175		}
   176	
   177		/*
   178		 * These who-am-i register mappings to model strings have been
   179		 * taken from the old /dev/freefall chardev and joystick driver:
   180		 * drivers/misc/lis3lv02d/lis3lv02d.c
   181		 */
   182		switch (smbus_data.byte) {
   183		case 0x32:
   184			type = "lis331dlh";
   185			break;
   186		case 0x33:
   187			type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
   188			break;
   189		case 0x3a:
   190			type = "lis3lv02dl_accel";
   191			break;
   192		case 0x3b:
   193			type = "lis302dl";
   194			break;
   195		default:
   196			dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
   197				 smbus_data.byte);
   198			return -ENODEV;
   199		}
   200	
   201		strscpy(info->type, type, I2C_NAME_SIZE);
   202		info->addr = addr;
   203		info->irq = smo8800->irq;
   204		info->swnode = &smo8800_accel_node;
   205		return 0;
   206	}
   207
diff mbox series

Patch

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 7f7c9452a983..bb1d3e439761 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"
+#define LIS3_WHO_AM_I 0x0f
 
 #include <linux/device/bus.h>
 #include <linux/dmi.h>
@@ -24,6 +25,10 @@ 
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
+static bool use_misc_lis3lv02d;
+module_param(use_misc_lis3lv02d, bool, 0444);
+MODULE_PARM_DESC(use_misc_lis3lv02d, "Use /dev/freefall chardev + evdev joystick emulation instead of iio accel driver");
+
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
 	atomic_t counter;            /* count after last read */
@@ -141,6 +146,65 @@  static int smo8800_find_i801(struct device *dev, void *data)
 	return 1;
 }
 
+/*
+ * Set label to let iio-sensor-proxy know these freefall sensors are located in
+ * the laptop base (not the display) and are not intended for screen rotation.
+ */
+static const struct property_entry smo8800_accel_props[] = {
+	PROPERTY_ENTRY_STRING("label", "accel-base"),
+	{}
+};
+
+const struct software_node smo8800_accel_node = {
+	.properties = smo8800_accel_props,
+};
+
+static int smo8800_detect_accel(struct smo8800_device *smo8800,
+				struct i2c_adapter *adap, u8 addr,
+				struct i2c_board_info *info)
+{
+	union i2c_smbus_data smbus_data;
+	const char *type;
+	int err;
+
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0) {
+		dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
+		return err;
+	}
+
+	/*
+	 * These who-am-i register mappings to model strings have been
+	 * taken from the old /dev/freefall chardev and joystick driver:
+	 * drivers/misc/lis3lv02d/lis3lv02d.c
+	 */
+	switch (smbus_data.byte) {
+	case 0x32:
+		type = "lis331dlh";
+		break;
+	case 0x33:
+		type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
+		break;
+	case 0x3a:
+		type = "lis3lv02dl_accel";
+		break;
+	case 0x3b:
+		type = "lis302dl";
+		break;
+	default:
+		dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
+			 smbus_data.byte);
+		return -ENODEV;
+	}
+
+	strscpy(info->type, type, I2C_NAME_SIZE);
+	info->addr = addr;
+	info->irq = smo8800->irq;
+	info->swnode = &smo8800_accel_node;
+	return 0;
+}
+
 /*
  * Accelerometer's I2C address is not specified in DMI nor ACPI,
  * so it is needed to define mapping table based on DMI product names.
@@ -174,7 +238,7 @@  static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 	struct i2c_adapter *adap = NULL;
 	const char *dmi_product_name;
 	u8 addr = 0;
-	int i;
+	int i, err;
 
 	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
 	if (!adap)
@@ -195,9 +259,19 @@  static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 		goto put_adapter;
 	}
 
-	info.addr = addr;
-	info.irq = smo8800->irq;
-	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+	/* Always detect the accel-type, this also checks the accel is actually there */
+	err = smo8800_detect_accel(smo8800, adap, addr, &info);
+	if (err)
+		goto put_adapter;
+
+	/*
+	 * If requested override detected type with "lis3lv02d" i2c_client_id,
+	 * for the old drivers/misc/lis3lv02d/lis3lv02d.c driver.
+	 */
+	if (use_misc_lis3lv02d) {
+		strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+		info.swnode = NULL;
+	}
 
 	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
 	if (IS_ERR(smo8800->i2c_dev)) {