| Message ID | 20231224213629.395741-6-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
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?
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
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 > >
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.
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.
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!)
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
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 --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)) {
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(-)