| Message ID | 20231224213629.395741-4-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:19 Hans de Goede wrote: > +static int smo8800_find_i801(struct device *dev, void *data) > +{ > + 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'm not happy with seeing another hardcoded list of device ids in the driver. Are not we able to find compatible i801 adapter without need to hardcode this list there in smo driver? And if really not, what about sharing (or exporting) list from the i801 driver? I'm just worried that this PCI id list will increase in the future and it would be required to add a new and new id into it... > + struct i2c_adapter *adap, **adap_ret = data; > + struct pci_dev *pdev; > + int i; > + > + adap = i2c_verify_adapter(dev); > + if (!adap) > + return 0; > + > + if (!strstarts(adap->name, "SMBus I801 adapter")) > + return 0; > + > + /* The parent of an I801 adapter is always a PCI device */ > + pdev = to_pci_dev(adap->dev.parent); > + for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { > + if (pdev->device == i801_idf_pci_device_ids[i]) > + return 0; /* Only register client on main SMBus channel */ > + } > + > + *adap_ret = i2c_get_adapter(adap->nr); > + return 1; > +}
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.
Hi Pali, On 12/24/23 22:55, Pali Rohár wrote: > On Sunday 24 December 2023 22:36:19 Hans de Goede wrote: >> +static int smo8800_find_i801(struct device *dev, void *data) >> +{ >> + 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'm not happy with seeing another hardcoded list of device ids in the > driver. Are not we able to find compatible i801 adapter without need to > hardcode this list there in smo driver? I agree that having this hardcoded is not ideal. The problem is that for a couple of generations (Patsburg is for Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell) intel had multiple i2c-i801 controllers / I2C-busses in the PCH and the i2c_client needs to be instantiated on the primary i2c-i801 (compatible) controller. Luckily Intel has only done this for these 2 generations PCH all older and newer PCHs only have 1 i2c-i801 I2C bus. So even though having this hardcoded is not ideal, the list should never change since it is only for this 2 somewhat old PCH generations and new generations are not impacted. So I believe that this is the best solution. Regards, Hans > And if really not, what about > sharing (or exporting) list from the i801 driver? > > I'm just worried that this PCI id list will increase in the future and > it would be required to add a new and new id into it... > >> + struct i2c_adapter *adap, **adap_ret = data; >> + struct pci_dev *pdev; >> + int i; >> + >> + adap = i2c_verify_adapter(dev); >> + if (!adap) >> + return 0; >> + >> + if (!strstarts(adap->name, "SMBus I801 adapter")) >> + return 0; >> + >> + /* The parent of an I801 adapter is always a PCI device */ >> + pdev = to_pci_dev(adap->dev.parent); >> + for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { >> + if (pdev->device == i801_idf_pci_device_ids[i]) >> + return 0; /* Only register client on main SMBus channel */ >> + } >> + >> + *adap_ret = i2c_get_adapter(adap->nr); >> + return 1; >> +} >
On Friday 05 January 2024 17:31:32 Hans de Goede wrote: > Hi Pali, > > On 12/24/23 22:55, Pali Rohár wrote: > > On Sunday 24 December 2023 22:36:19 Hans de Goede wrote: > >> +static int smo8800_find_i801(struct device *dev, void *data) > >> +{ > >> + 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'm not happy with seeing another hardcoded list of device ids in the > > driver. Are not we able to find compatible i801 adapter without need to > > hardcode this list there in smo driver? > > I agree that having this hardcoded is not ideal. > > The problem is that for a couple of generations (Patsburg is for > Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell) > intel had multiple i2c-i801 controllers / I2C-busses in the PCH > and the i2c_client needs to be instantiated on the primary > i2c-i801 (compatible) controller. > > Luckily Intel has only done this for these 2 generations PCH > all older and newer PCHs only have 1 i2c-i801 I2C bus. > > So even though having this hardcoded is not ideal, > the list should never change since it is only for > this 2 somewhat old PCH generations and new generations > are not impacted. So I believe that this is the best > solution. I see. Seems that this is the best solution which we have. Anyway, is not possible to use pci_dev_driver() to find i801 driver and from it takes that list of devices which have FEATURE_IDF flag? I have looked at the code only quickly and maybe it could be possible. Just an idea. > Regards, > > Hans > > > > > > > And if really not, what about > > sharing (or exporting) list from the i801 driver? > > > > I'm just worried that this PCI id list will increase in the future and > > it would be required to add a new and new id into it... > > > >> + struct i2c_adapter *adap, **adap_ret = data; > >> + struct pci_dev *pdev; > >> + int i; > >> + > >> + adap = i2c_verify_adapter(dev); > >> + if (!adap) > >> + return 0; > >> + > >> + if (!strstarts(adap->name, "SMBus I801 adapter")) > >> + return 0; > >> + > >> + /* The parent of an I801 adapter is always a PCI device */ > >> + pdev = to_pci_dev(adap->dev.parent); > >> + for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { > >> + if (pdev->device == i801_idf_pci_device_ids[i]) > >> + return 0; /* Only register client on main SMBus channel */ > >> + } > >> + > >> + *adap_ret = i2c_get_adapter(adap->nr); > >> + return 1; > >> +} > > >
Hi, On 1/5/24 19:26, Pali Rohár wrote: > On Friday 05 January 2024 17:31:32 Hans de Goede wrote: >> Hi Pali, >> >> On 12/24/23 22:55, Pali Rohár wrote: >>> On Sunday 24 December 2023 22:36:19 Hans de Goede wrote: >>>> +static int smo8800_find_i801(struct device *dev, void *data) >>>> +{ >>>> + 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'm not happy with seeing another hardcoded list of device ids in the >>> driver. Are not we able to find compatible i801 adapter without need to >>> hardcode this list there in smo driver? >> >> I agree that having this hardcoded is not ideal. >> >> The problem is that for a couple of generations (Patsburg is for >> Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell) >> intel had multiple i2c-i801 controllers / I2C-busses in the PCH >> and the i2c_client needs to be instantiated on the primary >> i2c-i801 (compatible) controller. >> >> Luckily Intel has only done this for these 2 generations PCH >> all older and newer PCHs only have 1 i2c-i801 I2C bus. >> >> So even though having this hardcoded is not ideal, >> the list should never change since it is only for >> this 2 somewhat old PCH generations and new generations >> are not impacted. So I believe that this is the best >> solution. > > I see. Seems that this is the best solution which we have. > > Anyway, is not possible to use pci_dev_driver() to find i801 driver and > from it takes that list of devices which have FEATURE_IDF flag? I have > looked at the code only quickly and maybe it could be possible. Just an > idea. That is an interesting idea, but ... that would mean interpreting the driver_data set by the i2c-i801 driver inside the dell-smo8800 code, so this would basically rely on the meaning of that driver_data never changing. I would rather just duplicate the 6 PCI ids and decouple the 2 drivers. Regards, Hans >>>> + struct i2c_adapter *adap, **adap_ret = data; >>>> + struct pci_dev *pdev; >>>> + int i; >>>> + >>>> + adap = i2c_verify_adapter(dev); >>>> + if (!adap) >>>> + return 0; >>>> + >>>> + if (!strstarts(adap->name, "SMBus I801 adapter")) >>>> + return 0; >>>> + >>>> + /* The parent of an I801 adapter is always a PCI device */ >>>> + pdev = to_pci_dev(adap->dev.parent); >>>> + for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { >>>> + if (pdev->device == i801_idf_pci_device_ids[i]) >>>> + return 0; /* Only register client on main SMBus channel */ >>>> + } >>>> + >>>> + *adap_ret = i2c_get_adapter(adap->nr); >>>> + return 1; >>>> +} >>> >> >
On Saturday 06 January 2024 13:13:01 Hans de Goede wrote: > Hi, > > On 1/5/24 19:26, Pali Rohár wrote: > > On Friday 05 January 2024 17:31:32 Hans de Goede wrote: > >> Hi Pali, > >> > >> On 12/24/23 22:55, Pali Rohár wrote: > >>> On Sunday 24 December 2023 22:36:19 Hans de Goede wrote: > >>>> +static int smo8800_find_i801(struct device *dev, void *data) > >>>> +{ > >>>> + 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'm not happy with seeing another hardcoded list of device ids in the > >>> driver. Are not we able to find compatible i801 adapter without need to > >>> hardcode this list there in smo driver? > >> > >> I agree that having this hardcoded is not ideal. > >> > >> The problem is that for a couple of generations (Patsburg is for > >> Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell) > >> intel had multiple i2c-i801 controllers / I2C-busses in the PCH > >> and the i2c_client needs to be instantiated on the primary > >> i2c-i801 (compatible) controller. > >> > >> Luckily Intel has only done this for these 2 generations PCH > >> all older and newer PCHs only have 1 i2c-i801 I2C bus. > >> > >> So even though having this hardcoded is not ideal, > >> the list should never change since it is only for > >> this 2 somewhat old PCH generations and new generations > >> are not impacted. So I believe that this is the best > >> solution. > > > > I see. Seems that this is the best solution which we have. > > > > Anyway, is not possible to use pci_dev_driver() to find i801 driver and > > from it takes that list of devices which have FEATURE_IDF flag? I have > > looked at the code only quickly and maybe it could be possible. Just an > > idea. > > That is an interesting idea, but ... > > that would mean interpreting the driver_data set by the i2c-i801 > driver inside the dell-smo8800 code, so this would basically rely on > the meaning of that driver_data never changing. I would rather just > duplicate the 6 PCI ids and decouple the 2 drivers. It was just an alternative idea. In case of code duplication I would suggest to write a comment on both places (into i801 and smo drivers) that this information is duplicated in both drivers and should be synchronized in case somebody would need to modify it at either place. > Regards, > > Hans > > > > > >>>> + struct i2c_adapter *adap, **adap_ret = data; > >>>> + struct pci_dev *pdev; > >>>> + int i; > >>>> + > >>>> + adap = i2c_verify_adapter(dev); > >>>> + if (!adap) > >>>> + return 0; > >>>> + > >>>> + if (!strstarts(adap->name, "SMBus I801 adapter")) > >>>> + return 0; > >>>> + > >>>> + /* The parent of an I801 adapter is always a PCI device */ > >>>> + pdev = to_pci_dev(adap->dev.parent); > >>>> + for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { > >>>> + if (pdev->device == i801_idf_pci_device_ids[i]) > >>>> + return 0; /* Only register client on main SMBus channel */ > >>>> + } > >>>> + > >>>> + *adap_ret = i2c_get_adapter(adap->nr); > >>>> + return 1; > >>>> +} > >>> > >> > > >
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-4-hdegoede%40redhat.com patch subject: [PATCH 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 config: i386-randconfig-003-20240106 (https://download.01.org/0day-ci/archive/20240108/202401081755.B1WHdRuF-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/20240108/202401081755.B1WHdRuF-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/202401081755.B1WHdRuF-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:278: 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:179: undefined reference to `i2c_bus_type' >> ld: drivers/platform/x86/dell/dell-smo8800.c:211: undefined reference to `i2c_put_adapter' ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_probe': drivers/platform/x86/dell/dell-smo8800.c:267: 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:201: undefined reference to `i2c_new_client_device' ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_find_i801': >> drivers/platform/x86/dell/dell-smo8800.c:126: undefined reference to `i2c_verify_adapter' >> ld: drivers/platform/x86/dell/dell-smo8800.c:140: undefined reference to `i2c_get_adapter' vim +278 drivers/platform/x86/dell/dell-smo8800.c 111 112 static int smo8800_find_i801(struct device *dev, void *data) 113 { 114 static const u16 i801_idf_pci_device_ids[] = { 115 0x1d70, /* Patsburg (PCH) */ 116 0x1d71, /* Patsburg (PCH) */ 117 0x1d72, /* Patsburg (PCH) */ 118 0x8d7d, /* Wellsburg (PCH) */ 119 0x8d7e, /* Wellsburg (PCH) */ 120 0x8d7f, /* Wellsburg (PCH) */ 121 }; 122 struct i2c_adapter *adap, **adap_ret = data; 123 struct pci_dev *pdev; 124 int i; 125 > 126 adap = i2c_verify_adapter(dev); 127 if (!adap) 128 return 0; 129 130 if (!strstarts(adap->name, "SMBus I801 adapter")) 131 return 0; 132 133 /* The parent of an I801 adapter is always a PCI device */ 134 pdev = to_pci_dev(adap->dev.parent); 135 for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { 136 if (pdev->device == i801_idf_pci_device_ids[i]) 137 return 0; /* Only register client on main SMBus channel */ 138 } 139 > 140 *adap_ret = i2c_get_adapter(adap->nr); 141 return 1; 142 } 143 144 /* 145 * Accelerometer's I2C address is not specified in DMI nor ACPI, 146 * so it is needed to define mapping table based on DMI product names. 147 */ 148 static const struct { 149 const char *dmi_product_name; 150 unsigned short i2c_addr; 151 } dell_lis3lv02d_devices[] = { 152 /* 153 * Dell platform team told us that these Latitude devices have 154 * ST microelectronics accelerometer at I2C address 0x29. 155 */ 156 { "Latitude E5250", 0x29 }, 157 { "Latitude E5450", 0x29 }, 158 { "Latitude E5550", 0x29 }, 159 { "Latitude E6440", 0x29 }, 160 { "Latitude E6440 ATG", 0x29 }, 161 { "Latitude E6540", 0x29 }, 162 /* 163 * Additional individual entries were added after verification. 164 */ 165 { "Latitude 5480", 0x29 }, 166 { "Latitude E6330", 0x29 }, 167 { "Vostro V131", 0x1d }, 168 { "Vostro 5568", 0x29 }, 169 }; 170 171 static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) 172 { 173 struct i2c_board_info info = { }; 174 struct i2c_adapter *adap = NULL; 175 const char *dmi_product_name; 176 u8 addr = 0; 177 int i; 178 > 179 bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801); 180 if (!adap) 181 return; 182 183 dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); 184 for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { 185 if (strcmp(dmi_product_name, 186 dell_lis3lv02d_devices[i].dmi_product_name) == 0) { 187 addr = dell_lis3lv02d_devices[i].i2c_addr; 188 break; 189 } 190 } 191 192 if (!addr) { 193 dev_warn(smo8800->dev, 194 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); 195 goto put_adapter; 196 } 197 198 info.addr = addr; 199 strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); 200 201 smo8800->i2c_dev = i2c_new_client_device(adap, &info); 202 if (IS_ERR(smo8800->i2c_dev)) { 203 dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev), 204 "registering accel i2c_client\n"); 205 smo8800->i2c_dev = NULL; 206 } else { 207 dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", 208 info.type, info.addr); 209 } 210 put_adapter: > 211 i2c_put_adapter(adap); 212 } 213 214 static int smo8800_probe(struct platform_device *device) 215 { 216 int err; 217 struct smo8800_device *smo8800; 218 219 if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) 220 return false; 221 222 smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL); 223 if (!smo8800) { 224 dev_err(&device->dev, "failed to allocate device data\n"); 225 return -ENOMEM; 226 } 227 228 smo8800->dev = &device->dev; 229 smo8800->miscdev.minor = MISC_DYNAMIC_MINOR; 230 smo8800->miscdev.name = "freefall"; 231 smo8800->miscdev.fops = &smo8800_misc_fops; 232 233 init_waitqueue_head(&smo8800->misc_wait); 234 235 err = platform_get_irq(device, 0); 236 if (err < 0) 237 return err; 238 smo8800->irq = err; 239 240 smo8800_instantiate_i2c_client(smo8800); 241 242 err = misc_register(&smo8800->miscdev); 243 if (err) { 244 dev_err(&device->dev, "failed to register misc dev: %d\n", err); 245 goto error_unregister_i2c_client; 246 } 247 248 err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, 249 smo8800_interrupt_thread, 250 IRQF_TRIGGER_RISING | IRQF_ONESHOT, 251 DRIVER_NAME, smo8800); 252 if (err) { 253 dev_err(&device->dev, 254 "failed to request thread for IRQ %d: %d\n", 255 smo8800->irq, err); 256 goto error; 257 } 258 259 dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n", 260 smo8800->irq); 261 platform_set_drvdata(device, smo8800); 262 return 0; 263 264 error: 265 misc_deregister(&smo8800->miscdev); 266 error_unregister_i2c_client: 267 i2c_unregister_device(smo8800->i2c_dev); 268 return err; 269 } 270 271 static void smo8800_remove(struct platform_device *device) 272 { 273 struct smo8800_device *smo8800 = platform_get_drvdata(device); 274 275 free_irq(smo8800->irq, smo8800); 276 misc_deregister(&smo8800->miscdev); 277 dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > 278 i2c_unregister_device(smo8800->i2c_dev); 279 } 280
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 070999139c6d..552bad99f04d 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1141,125 +1141,6 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) } } -/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */ -static const char *const acpi_smo8800_ids[] = { - "SMO8800", - "SMO8801", - "SMO8810", - "SMO8811", - "SMO8820", - "SMO8821", - "SMO8830", - "SMO8831", -}; - -static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, - u32 nesting_level, - void *context, - void **return_value) -{ - struct acpi_device_info *info; - acpi_status status; - char *hid; - int i; - - status = acpi_get_object_info(obj_handle, &info); - if (ACPI_FAILURE(status)) - return AE_OK; - - if (!(info->valid & ACPI_VALID_HID)) - goto smo88xx_not_found; - - hid = info->hardware_id.string; - if (!hid) - goto smo88xx_not_found; - - i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid); - if (i < 0) - goto smo88xx_not_found; - - kfree(info); - - *return_value = NULL; - return AE_CTRL_TERMINATE; - -smo88xx_not_found: - kfree(info); - return AE_OK; -} - -static bool is_dell_system_with_lis3lv02d(void) -{ - void *err = ERR_PTR(-ENOENT); - - if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) - return false; - - /* - * Check that ACPI device SMO88xx is present and is functioning. - * Function acpi_get_devices() already filters all ACPI devices - * which are not present or are not functioning. - * ACPI device SMO88xx represents our ST microelectronics lis3lv02d - * accelerometer but unfortunately ACPI does not provide any other - * information (like I2C address). - */ - acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err); - - return !IS_ERR(err); -} - -/* - * Accelerometer's I2C address is not specified in DMI nor ACPI, - * so it is needed to define mapping table based on DMI product names. - */ -static const struct { - const char *dmi_product_name; - unsigned short i2c_addr; -} dell_lis3lv02d_devices[] = { - /* - * Dell platform team told us that these Latitude devices have - * ST microelectronics accelerometer at I2C address 0x29. - */ - { "Latitude E5250", 0x29 }, - { "Latitude E5450", 0x29 }, - { "Latitude E5550", 0x29 }, - { "Latitude E6440", 0x29 }, - { "Latitude E6440 ATG", 0x29 }, - { "Latitude E6540", 0x29 }, - /* - * Additional individual entries were added after verification. - */ - { "Latitude 5480", 0x29 }, - { "Vostro V131", 0x1d }, - { "Vostro 5568", 0x29 }, -}; - -static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) -{ - struct i2c_board_info info; - const char *dmi_product_name; - int i; - - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); - for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { - if (strcmp(dmi_product_name, - dell_lis3lv02d_devices[i].dmi_product_name) == 0) - break; - } - - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { - dev_warn(&priv->pci_dev->dev, - "Accelerometer lis3lv02d is present on SMBus but its" - " address is unknown, skipping registration\n"); - return; - } - - memset(&info, 0, sizeof(struct i2c_board_info)); - info.addr = dell_lis3lv02d_devices[i].i2c_addr; - strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); - i2c_new_client_device(&priv->adapter, &info); -} - /* Register optional slaves */ static void i801_probe_optional_slaves(struct i801_priv *priv) { @@ -1279,9 +1160,6 @@ static void i801_probe_optional_slaves(struct i801_priv *priv) if (dmi_name_in_vendors("FUJITSU")) dmi_walk(dmi_check_onboard_devices, &priv->adapter); - if (is_dell_system_with_lis3lv02d()) - register_dell_lis3lv02d_i2c_device(priv); - /* Instantiate SPD EEPROMs unless the SMBus is multiplexed */ #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) if (!priv->mux_pdev) diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c index 86f898a857e1..416b399cb9a1 100644 --- a/drivers/platform/x86/dell/dell-smo8800.c +++ b/drivers/platform/x86/dell/dell-smo8800.c @@ -4,19 +4,23 @@ * * Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com> * Copyright (C) 2014 Pali Rohár <pali@kernel.org> + * Copyright (C) 2023 Hans de Goede <hansg@kernel.org> * * This is loosely based on lis3lv02d driver. */ #define DRIVER_NAME "smo8800" +#include <linux/device/bus.h> #include <linux/dmi.h> #include <linux/fs.h> +#include <linux/i2c.h> #include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/miscdevice.h> #include <linux/mod_devicetable.h> #include <linux/module.h> +#include <linux/pci.h> #include <linux/platform_device.h> #include <linux/uaccess.h> @@ -27,6 +31,7 @@ struct smo8800_device { unsigned long misc_opened; /* whether the device is open */ wait_queue_head_t misc_wait; /* Wait queue for the misc dev */ struct device *dev; /* acpi device */ + struct i2c_client *i2c_dev; /* i2c_client for lis3lv02d */ }; static irqreturn_t smo8800_interrupt_quick(int irq, void *data) @@ -104,6 +109,108 @@ static const struct file_operations smo8800_misc_fops = { .release = smo8800_misc_release, }; +static int smo8800_find_i801(struct device *dev, void *data) +{ + 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) */ + }; + struct i2c_adapter *adap, **adap_ret = data; + struct pci_dev *pdev; + int i; + + adap = i2c_verify_adapter(dev); + if (!adap) + return 0; + + if (!strstarts(adap->name, "SMBus I801 adapter")) + return 0; + + /* The parent of an I801 adapter is always a PCI device */ + pdev = to_pci_dev(adap->dev.parent); + for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) { + if (pdev->device == i801_idf_pci_device_ids[i]) + return 0; /* Only register client on main SMBus channel */ + } + + *adap_ret = i2c_get_adapter(adap->nr); + return 1; +} + +/* + * Accelerometer's I2C address is not specified in DMI nor ACPI, + * so it is needed to define mapping table based on DMI product names. + */ +static const struct { + const char *dmi_product_name; + unsigned short i2c_addr; +} dell_lis3lv02d_devices[] = { + /* + * Dell platform team told us that these Latitude devices have + * ST microelectronics accelerometer at I2C address 0x29. + */ + { "Latitude E5250", 0x29 }, + { "Latitude E5450", 0x29 }, + { "Latitude E5550", 0x29 }, + { "Latitude E6440", 0x29 }, + { "Latitude E6440 ATG", 0x29 }, + { "Latitude E6540", 0x29 }, + /* + * Additional individual entries were added after verification. + */ + { "Latitude 5480", 0x29 }, + { "Latitude E6330", 0x29 }, + { "Vostro V131", 0x1d }, + { "Vostro 5568", 0x29 }, +}; + +static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) +{ + struct i2c_board_info info = { }; + struct i2c_adapter *adap = NULL; + const char *dmi_product_name; + u8 addr = 0; + int i; + + bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801); + if (!adap) + return; + + dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { + if (strcmp(dmi_product_name, + dell_lis3lv02d_devices[i].dmi_product_name) == 0) { + addr = dell_lis3lv02d_devices[i].i2c_addr; + break; + } + } + + if (!addr) { + dev_warn(smo8800->dev, + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); + goto put_adapter; + } + + info.addr = addr; + strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); + + smo8800->i2c_dev = i2c_new_client_device(adap, &info); + if (IS_ERR(smo8800->i2c_dev)) { + dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev), + "registering accel i2c_client\n"); + smo8800->i2c_dev = NULL; + } else { + dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", + info.type, info.addr); + } +put_adapter: + i2c_put_adapter(adap); +} + static int smo8800_probe(struct platform_device *device) { int err; @@ -130,10 +237,12 @@ static int smo8800_probe(struct platform_device *device) return err; smo8800->irq = err; + smo8800_instantiate_i2c_client(smo8800); + err = misc_register(&smo8800->miscdev); if (err) { dev_err(&device->dev, "failed to register misc dev: %d\n", err); - return err; + goto error_unregister_i2c_client; } err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, @@ -154,6 +263,8 @@ static int smo8800_probe(struct platform_device *device) error: misc_deregister(&smo8800->miscdev); +error_unregister_i2c_client: + i2c_unregister_device(smo8800->i2c_dev); return err; } @@ -164,9 +275,9 @@ static void smo8800_remove(struct platform_device *device) free_irq(smo8800->irq, smo8800); misc_deregister(&smo8800->miscdev); dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); + i2c_unregister_device(smo8800->i2c_dev); } -/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */ static const struct acpi_device_id smo8800_ids[] = { { "SMO8800", 0 }, { "SMO8801", 0 }, @@ -193,3 +304,5 @@ module_platform_driver(smo8800_driver); MODULE_DESCRIPTION("Dell Latitude freefall driver (ACPI SMO88XX)"); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Sonal Santan, Pali Rohár"); +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */ +MODULE_SOFTDEP("pre: i2c-i801");
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/i2c/busses/i2c-i801.c | 122 ----------------------- drivers/platform/x86/dell/dell-smo8800.c | 117 +++++++++++++++++++++- 2 files changed, 115 insertions(+), 124 deletions(-)