Hi Pali,
On 6/22/24 6:35 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 17:12:50 Pali Rohár wrote:
>> On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
>>> Hi Pali,
>>>
>>> On 6/22/24 4:20 PM, Pali Rohár wrote:
>>>> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>>>>> Hi Pali,
>>>>>
>>>>> On 6/22/24 3:16 PM, Pali Rohár wrote:
>>>>>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>>>>>>> It is not necessary to handle the Dell specific instantiation of
>>>>>>> i2c_client-s for SMO88xx 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 SMO88xx specific dell-smo8800 driver.
>>>>>>
>>>>>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
>>>>>> and freefall code for smo88xx are basically independent.
>>>>>>
>>>>>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
>>>>>> detection. The only thing which have these "drivers" common is the ACPI
>>>>>> detection mechanism based on presence of SMO88?? identifiers from
>>>>>> acpi_smo8800_ids[] array.
>>>>>>
>>>>>> I think it makes both "drivers" cleaner if they are put into separate
>>>>>> files as they are independent of each one.
>>>>>>
>>>>>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
>>>>>> instead (or similar name)? And just share list of ACPI ids via some
>>>>>> header file (or something like that).
>>>>>
>>>>> Interesting idea, but that will not work, only 1 driver can bind to
>>>>> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
>>>>
>>>> And it is required to bind lis3 device to ACPI code? What is needed is
>>>> just to check if system matches DMI strings and ACPI strings. You are
>>>> not binding device to DMI strings, so I think there is no need to bind
>>>> it neither to ACPI strings.
>>>
>>> The driver needs to bind to something ...
>>
>> Why?
>>
>> Currently in i2c-i801.c file was called just
>> register_dell_lis3lv02d_i2c_device() function and there was no binding
>> to anything, no binding to DMI structure and neither no binding to ACPI
>> structures.
>>
>> And if I'm looking correctly at your new function
>> smo8800_instantiate_i2c_client() it does not bind device neither.
>> And smo8800_i2c_bus_notify() does not depend on binding.
>>
>> So I do not see where is that binding requirement.
>
> Now I have tried to do it, to move code into dell-lis3lv02d.c file.
>
> Below is example how it could look like. I reused most of your code.
> I have not tested it. It is just an idea.
Thank you for going through the trouble of writing this proof
of concept.
The problem with DMI matching, instead of binding to the ACPI
SMO88xx platform_device is that this will now only load on
laptops for which we already have the DMI ids.
So this looses the warning about the i2c address info missing
which we currently give when there is a SMO88xx ACPI device
on laptops not in the DMI table (the current i2c-i801.c code
has this already). Not giving the warning in turn means we
cannot inform users about trying the new probe option, which is
the whole reason to do this patch-set in the first place.
I still believe that keeping this new code in dell-smo8800.c
is best:
1. This very much is about handling the SMO88xx ACPI devices
which makes putting it in the smo8800.c driver the logical /
the right thing to do.
2. This only adds 400 lines of code. After this all of
dell-smo8800.c is only 600 lines. That is very small so
I really so no pressing need to spread this out over 2 files.
3. You claim the freefall IRQ and the i2c_client instantiation
are 2 different things, but they are both for the same chip
and normally would both be described in the same ACPI device
node. The manual i2c_client instantation is done to address
a shortcoming of the SMO88xx ACPI device node, so handling it
belongs in the smo8800 driver.
Regards,
Hans
> #include <linux/acpi.h>
> #include <linux/dmi.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/notifier.h>
> #include <linux/workqueue.h>
>
> static struct work_struct dell_lis3lv02d_i2c_work;
> static struct i2c_client *dell_lis3lv02d_i2c_dev;
> static unsigned short dell_lis3lv02d_i2c_addr;
>
> static int dell_lis3lv02d_find_i801(struct device *dev, void *data)
> {
> struct i2c_adapter *adap, **adap_ret = data;
>
> adap = i2c_verify_adapter(dev);
> if (!adap)
> return 0;
>
> if (!strstarts(adap->name, "SMBus I801 adapter"))
> return 0;
>
> *adap_ret = i2c_get_adapter(adap->nr);
> return 1;
> }
>
> static void dell_lis3lv02d_instantiate_i2c_client(struct work_struct *work)
> {
> struct i2c_board_info info = { };
> struct i2c_adapter *adap = NULL;
> struct i2c_client *client;
>
> if (dell_lis3lv02d_i2c_dev)
> return;
>
> bus_for_each_dev(&i2c_bus_type, NULL, &adap, dell_lis3lv02d_find_i801);
> if (!adap)
> return;
>
> info.addr = dell_lis3lv02d_i2c_addr;
> strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>
> client = i2c_new_client_device(adap, &info);
> if (IS_ERR(client)) {
> pr_err("error %ld registering %s i2c_client\n",
> PTR_ERR(client), info.type);
> return;
> }
>
> dell_lis3lv02d_i2c_dev = client;
>
> pr_err("registered %s i2c_client on address 0x%02x\n",
> info.type, info.addr);
> }
>
> static int dell_lis3lv02d_i2c_bus_notify(struct notifier_block *nb,
> unsigned long action, void *data)
> {
> struct device *dev = data;
> struct i2c_client *client;
> struct i2c_adapter *adap;
>
> switch (action) {
> case BUS_NOTIFY_ADD_DEVICE:
> adap = i2c_verify_adapter(dev);
> if (!adap)
> break;
>
> if (strstarts(adap->name, "SMBus I801 adapter"))
> queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
> break;
> case BUS_NOTIFY_REMOVED_DEVICE:
> client = i2c_verify_client(dev);
> if (!client)
> break;
>
> if (dell_lis3lv02d_i2c_dev == client) {
> pr_debug("accelerometer i2c_client removed\n");
> dell_lis3lv02d_i2c_dev = NULL;
> }
> break;
> default:
> break;
> }
>
> return 0;
> }
>
> // TODO: move this array into dell-smo8800.h header file
> static const char *const acpi_smo8800_ids[] __initconst = {
> "SMO8800",
> "SMO8801",
> "SMO8810",
> "SMO8811",
> "SMO8820",
> "SMO8821",
> "SMO8830",
> "SMO8831",
> };
>
> static acpi_status __init 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 __init has_acpi_smo88xx_device(void)
> {
> void *err = ERR_PTR(-ENOENT);
>
> 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 dmi_system_id dell_lis3lv02d_devices[] __initconst = {
> /*
> * Dell platform team told us that these Latitude devices have
> * ST microelectronics accelerometer at I2C address 0x29.
> */
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> },
> .driver_data = (void *)0x29,
> },
> /*
> * Additional individual entries were added after verification.
> */
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> },
> .driver_data = (void *)0x1d,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> },
> .driver_data = (void *)0x29,
> },
> {
> .matches = {
> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> },
> .driver_data = (void *)0x29,
> },
> { }
> };
> MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);
>
> static struct notifier_block dell_lis3lv02d_i2c_nb = {
> .notifier_call = dell_lis3lv02d_i2c_bus_notify,
> };
>
> static int __init dell_lis3lv02d_init(void)
> {
> const struct dmi_system_id *lis3lv02d_dmi_id;
> int err;
>
> if (!has_acpi_smo88xx_device())
> return -ENODEV;
>
> lis3lv02d_dmi_id = dmi_first_match(dell_lis3lv02d_devices);
> if (!lis3lv02d_dmi_id)
> return -ENODEV;
>
> dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;
>
> err = bus_register_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
> if (err)
> return err;
>
> INIT_WORK(&dell_lis3lv02d_i2c_work, dell_lis3lv02d_instantiate_i2c_client);
> queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
>
> return 0;
> }
>
> static void __exit dell_lis3lv02d_exit(void)
> {
> bus_unregister_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
> cancel_work_sync(&dell_lis3lv02d_i2c_work);
> if (dell_lis3lv02d_i2c_dev)
> i2c_unregister_device(dell_lis3lv02d_i2c_dev);
> }
>
> module_init(dell_lis3lv02d_init);
> module_exit(dell_lis3lv02d_exit);
>
> MODULE_LICENSE("GPL");
>
